Refactor FeedbackData and add tests.

Refactor FeedbackData and add tests.

BUG=341554
TEST=unittests, manual feedback send from debug chrome build
TBR=jochen for OWNERS

[achaulk, 2014-05-23 14:27:41]

Hopefully this won't cause weird crashes this time around

[achaulk, 2014-05-27 19:08:28]

ping

[gauravsh, 2014-05-27 23:23:07]

Will rkc/zork be better reviewers for this code?

[achaulk, 2014-05-28 16:51:45]

Oh right, I forgot to add one of them +zork

[Zachary Kuznia, 2014-05-28 21:39:36]

https://codereview.chromium.org/296173003/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/feedback_private/feedback_service.cc (right):

https://codereview.chromium.org/296173003/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/feedback_private/feedback_service.cc:77: if
(data.get())
just use:
if (data)

https://codereview.chromium.org/296173003/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/feedback_private/feedback_service.cc:86: if
(data.get())
ditto

https://codereview.chromium.org/296173003/diff/1/components/feedback/feedback...
File components/feedback/feedback_common.cc (right):

https://codereview.chromium.org/296173003/diff/1/components/feedback/feedback...
components/feedback/feedback_common.cc:64: if (key == "" || value == "")
key.empty() || value.empty()

https://codereview.chromium.org/296173003/diff/1/components/feedback/feedback...
components/feedback/feedback_common.cc:126: scoped_ptr<std::string> data) {
Why doesn't this take const std::string& for data?  Alternately, why not make
file->data a scoped_ptr?  It seems very counter-intuitive to pass ownership of a
pointer, then copy the string.

file->data.swap(*(data.get())); is another better solution.

https://codereview.chromium.org/296173003/diff/1/components/feedback/feedback...
components/feedback/feedback_common.cc:141: if (logs_.get()) {
if (logs_)

https://codereview.chromium.org/296173003/diff/1/components/feedback/feedback...
components/feedback/feedback_common.cc:142: for
(FeedbackCommon::SystemLogsMap::const_iterator i = logs->begin();
Why not logs_->insert(logs->begin(), logs->end()); ?

https://codereview.chromium.org/296173003/diff/1/components/feedback/feedback...
File components/feedback/feedback_common.h (right):

https://codereview.chromium.org/296173003/diff/1/components/feedback/feedback...
components/feedback/feedback_common.h:67: std::string* image() const { return
image_.get(); }
const?

https://codereview.chromium.org/296173003/diff/1/components/feedback/feedback...
components/feedback/feedback_common.h:68: SystemLogsMap* sys_info() const {
return logs_.get(); }
const?

https://codereview.chromium.org/296173003/diff/1/components/feedback/feedback...
File components/feedback/feedback_data.cc (right):

https://codereview.chromium.org/296173003/diff/1/components/feedback/feedback...
components/feedback/feedback_data.cc:41: pending_op_count_--;
-- should precede variable.

Repeat throughout this file.

https://codereview.chromium.org/296173003/diff/1/components/feedback/feedback...
File components/feedback/feedback_util.cc (right):

https://codereview.chromium.org/296173003/diff/1/components/feedback/feedback...
components/feedback/feedback_util.cc:102: }  // feedback_util
This comment is wrong.

[achaulk, 2014-05-29 17:22:29]

https://codereview.chromium.org/296173003/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/feedback_private/feedback_service.cc (right):

https://codereview.chromium.org/296173003/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/feedback_private/feedback_service.cc:77: if
(data.get())
On 2014/05/28 21:39:36, Zachary Kuznia wrote:
> just use:
> if (data)

Done.

https://codereview.chromium.org/296173003/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/feedback_private/feedback_service.cc:86: if
(data.get())
On 2014/05/28 21:39:36, Zachary Kuznia wrote:
> ditto

Done.

https://codereview.chromium.org/296173003/diff/1/components/feedback/feedback...
File components/feedback/feedback_common.cc (right):

https://codereview.chromium.org/296173003/diff/1/components/feedback/feedback...
components/feedback/feedback_common.cc:64: if (key == "" || value == "")
On 2014/05/28 21:39:36, Zachary Kuznia wrote:
> key.empty() || value.empty()

Done.

https://codereview.chromium.org/296173003/diff/1/components/feedback/feedback...
components/feedback/feedback_common.cc:126: scoped_ptr<std::string> data) {
On 2014/05/28 21:39:36, Zachary Kuznia wrote:
> Why doesn't this take const std::string& for data?  Alternately, why not make
> file->data a scoped_ptr?  It seems very counter-intuitive to pass ownership of
a
> pointer, then copy the string.
> 
> file->data.swap(*(data.get())); is another better solution.

I changed the struct to have a scoped_ptr, oversight on my part

https://codereview.chromium.org/296173003/diff/1/components/feedback/feedback...
components/feedback/feedback_common.cc:141: if (logs_.get()) {
On 2014/05/28 21:39:36, Zachary Kuznia wrote:
> if (logs_)

Done.

https://codereview.chromium.org/296173003/diff/1/components/feedback/feedback...
components/feedback/feedback_common.cc:142: for
(FeedbackCommon::SystemLogsMap::const_iterator i = logs->begin();
On 2014/05/28 21:39:36, Zachary Kuznia wrote:
> Why not logs_->insert(logs->begin(), logs->end()); ?

Yeah that works better

https://codereview.chromium.org/296173003/diff/1/components/feedback/feedback...
File components/feedback/feedback_common.h (right):

https://codereview.chromium.org/296173003/diff/1/components/feedback/feedback...
components/feedback/feedback_common.h:67: std::string* image() const { return
image_.get(); }
On 2014/05/28 21:39:36, Zachary Kuznia wrote:
> const?

Done.

https://codereview.chromium.org/296173003/diff/1/components/feedback/feedback...
components/feedback/feedback_common.h:68: SystemLogsMap* sys_info() const {
return logs_.get(); }
On 2014/05/28 21:39:36, Zachary Kuznia wrote:
> const?

Done.

https://codereview.chromium.org/296173003/diff/1/components/feedback/feedback...
File components/feedback/feedback_data.cc (right):

https://codereview.chromium.org/296173003/diff/1/components/feedback/feedback...
components/feedback/feedback_data.cc:41: pending_op_count_--;
On 2014/05/28 21:39:36, Zachary Kuznia wrote:
> -- should precede variable.
> 
> Repeat throughout this file.

Done.

https://codereview.chromium.org/296173003/diff/1/components/feedback/feedback...
File components/feedback/feedback_util.cc (right):

https://codereview.chromium.org/296173003/diff/1/components/feedback/feedback...
components/feedback/feedback_util.cc:102: }  // feedback_util
On 2014/05/28 21:39:36, Zachary Kuznia wrote:
> This comment is wrong.

Done.

[Zachary Kuznia, 2014-05-29 23:21:39]

lgtm

[achaulk, 2014-05-30 15:28:47]

The CQ bit was checked by achaulk@chromium.org

[commit-bot: I haz the power, 2014-05-30 15:30:01]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/achaulk@chromium.org/296173003/20001

[achaulk, 2014-05-30 18:18:06]

The CQ bit was unchecked by achaulk@chromium.org

[achaulk, 2014-05-30 18:18:43]

The CQ bit was checked by achaulk@chromium.org

[commit-bot: I haz the power, 2014-05-30 18:20:23]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/achaulk@chromium.org/296173003/40001

[achaulk, 2014-05-30 19:05:30]

The CQ bit was unchecked by achaulk@chromium.org

[achaulk, 2014-05-30 19:06:07]

The CQ bit was checked by achaulk@chromium.org

[commit-bot: I haz the power, 2014-05-30 19:08:35]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/achaulk@chromium.org/296173003/60001

[commit-bot: I haz the power, 2014-05-30 23:44:59]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/19...)
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/11229)

[commit-bot: I haz the power, 2014-05-30 23:52:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-30 23:52:17]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[achaulk, 2014-06-02 14:22:55]

The CQ bit was checked by achaulk@chromium.org

[commit-bot: I haz the power, 2014-06-02 14:23:27]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/achaulk@chromium.org/296173003/80001

[commit-bot: I haz the power, 2014-06-02 17:21:03]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/19...)
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)

[commit-bot: I haz the power, 2014-06-02 17:23:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-02 17:23:51]

Try jobs failed on following builders:
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/19...)
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)

[achaulk, 2014-06-02 17:56:17]

The CQ bit was checked by achaulk@chromium.org

[commit-bot: I haz the power, 2014-06-02 17:57:15]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/achaulk@chromium.org/296173003/100001

[achaulk, 2014-06-02 19:51:10]

Need owners reviews:
+yoz for chrome/browser/extensions/api/feedback_private/feedback_service.cc
+jochen for OWNERS/gyp files

[achaulk, 2014-06-02 19:51:12]

Need owners reviews:
+yoz for chrome/browser/extensions/api/feedback_private/feedback_service.cc
+jochen for OWNERS/gyp files

[Yoyo Zhou, 2014-06-02 19:55:29]

feedback_service LGTM

[achaulk, 2014-06-06 14:36:29]

The CQ bit was checked by achaulk@chromium.org

[commit-bot: I haz the power, 2014-06-06 14:37:19]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/achaulk@chromium.org/296173003/140001

[commit-bot: I haz the power, 2014-06-06 17:51:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-06 17:51:38]

Could not make sense out of svn commit message.

[achaulk, 2014-06-06 18:02:23]

The CQ bit was checked by achaulk@chromium.org

[achaulk, 2014-06-06 18:03:12]

The CQ bit was unchecked by achaulk@chromium.org