Move feedback files into src/components

Move some common feedback files into src/components

BUG=359299
TEST=build works

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=269152

[achaulk, 2014-04-07 19:52:01]

Per email thread with darin@ we are moving the feedback code into src/components
so we can use it in CrOS

[jochen (gone - plz use gerrit), 2014-04-08 07:45:35]

https://codereview.chromium.org/225183018/diff/1/components/feedback.gypi
File components/feedback.gypi (right):

https://codereview.chromium.org/225183018/diff/1/components/feedback.gypi#new...
components/feedback.gypi:9: 'type': '<(component)',
if you want to use <(component), you need to create a public API (something like
FEEDBACK_EXPORT macros)

https://codereview.chromium.org/225183018/diff/1/components/feedback.gypi#new...
components/feedback.gypi:19: 'feedback/feedback_report.cc',
please also list the header files

https://codereview.chromium.org/225183018/diff/1/components/feedback/OWNERS
File components/feedback/OWNERS (right):

https://codereview.chromium.org/225183018/diff/1/components/feedback/OWNERS#n...
components/feedback/OWNERS:5: 
no empty line

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

https://codereview.chromium.org/225183018/diff/1/components/feedback/feedback...
components/feedback/feedback_uploader.cc:5: #include "feedback_uploader.h"
please use a full path

https://codereview.chromium.org/225183018/diff/1/components/feedback/feedback...
components/feedback/feedback_uploader.cc:13: #include "feedback_report.h"
please use a full path

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

https://codereview.chromium.org/225183018/diff/1/components/feedback/feedback...
components/feedback/feedback_uploader.h:45: friend class FeedbackServiceTest;
please move the tests into the component as well

[achaulk, 2014-04-09 20:09:21]

After discussing with rkc@ we're going to move all of the feedback code into
here

https://codereview.chromium.org/225183018/diff/1/components/feedback.gypi
File components/feedback.gypi (right):

https://codereview.chromium.org/225183018/diff/1/components/feedback.gypi#new...
components/feedback.gypi:9: 'type': '<(component)',
On 2014/04/08 07:45:35, jochen wrote:
> if you want to use <(component), you need to create a public API (something
like
> FEEDBACK_EXPORT macros)

I have no idea what this does, I just copied it from another gyp file in here

https://codereview.chromium.org/225183018/diff/1/components/feedback.gypi#new...
components/feedback.gypi:19: 'feedback/feedback_report.cc',
On 2014/04/08 07:45:35, jochen wrote:
> please also list the header files

Done.

https://codereview.chromium.org/225183018/diff/1/components/feedback/OWNERS
File components/feedback/OWNERS (right):

https://codereview.chromium.org/225183018/diff/1/components/feedback/OWNERS#n...
components/feedback/OWNERS:5: 
On 2014/04/08 07:45:35, jochen wrote:
> no empty line

Done.

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

https://codereview.chromium.org/225183018/diff/1/components/feedback/feedback...
components/feedback/feedback_uploader.cc:5: #include "feedback_uploader.h"
On 2014/04/08 07:45:35, jochen wrote:
> please use a full path

Done.

https://codereview.chromium.org/225183018/diff/1/components/feedback/feedback...
components/feedback/feedback_uploader.cc:13: #include "feedback_report.h"
On 2014/04/08 07:45:35, jochen wrote:
> please use a full path

Done.

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

https://codereview.chromium.org/225183018/diff/1/components/feedback/feedback...
components/feedback/feedback_uploader.h:45: friend class FeedbackServiceTest;
Done

On 2014/04/08 07:45:35, jochen wrote:
> please move the tests into the component as well

[jochen (gone - plz use gerrit), 2014-04-10 07:42:49]

you'll have to restructure the code so it doesn't depend on chrome/

see e.g.
http://www.chromium.org/developers/design-documents/browser-components/cookbook

https://codereview.chromium.org/225183018/diff/20001/components/feedback.gypi
File components/feedback.gypi (right):

https://codereview.chromium.org/225183018/diff/20001/components/feedback.gypi...
components/feedback.gypi:39:
'../chrome/common/extensions/api/api.gyp:chrome_api',
components can't depend on chrome

https://codereview.chromium.org/225183018/diff/20001/components/feedback/DEPS
File components/feedback/DEPS (right):

https://codereview.chromium.org/225183018/diff/20001/components/feedback/DEPS...
components/feedback/DEPS:2: "+chrome/browser/browser_process.h",
no chrome

[achaulk, 2014-04-10 17:32:29]

https://codereview.chromium.org/225183018/diff/20001/components/feedback.gypi
File components/feedback.gypi (right):

https://codereview.chromium.org/225183018/diff/20001/components/feedback.gypi...
components/feedback.gypi:39:
'../chrome/common/extensions/api/api.gyp:chrome_api',
Done

On 2014/04/10 07:42:49, jochen wrote:
> components can't depend on chrome

https://codereview.chromium.org/225183018/diff/20001/components/feedback/DEPS
File components/feedback/DEPS (right):

https://codereview.chromium.org/225183018/diff/20001/components/feedback/DEPS...
components/feedback/DEPS:2: "+chrome/browser/browser_process.h",
On 2014/04/10 07:42:49, jochen wrote:
> no chrome

Done.

[achaulk, 2014-04-14 17:03:29]

ping

[jochen (gone - plz use gerrit), 2014-04-15 11:40:36]

On 2014/04/14 17:03:29, achaulk wrote:
> ping

Your component still depends on chrome. The main idea of components is not to
depend on chrome.

Please read up on what browser components are and how they're designed, and find
somebody local who can work with you to get the design of the component right.

[achaulk, 2014-04-15 13:37:36]

Oh I guess I forgot to remove the stale deps from the gyp files. Fixed

[achaulk, 2014-04-22 20:42:52]

ping

[jochen (gone - plz use gerrit), 2014-04-23 08:21:24]

https://codereview.chromium.org/225183018/diff/50001/components/feedback.gypi
File components/feedback.gypi (right):

https://codereview.chromium.org/225183018/diff/50001/components/feedback.gypi...
components/feedback.gypi:20: 'feedback/common/feedback_switches.cc',
unless you need to depend on those files from e.g. chrome/common I wouldn't put
those into a separate sub directory.

https://codereview.chromium.org/225183018/diff/50001/components/feedback.gypi...
components/feedback.gypi:33: 'target_name': 'feedback_internal',
what's the reason to introduce two targets?

https://codereview.chromium.org/225183018/diff/50001/components/feedback/DEPS
File components/feedback/DEPS (right):

https://codereview.chromium.org/225183018/diff/50001/components/feedback/DEPS...
components/feedback/DEPS:8: "+third_party/zlib/google/zip.h",
you shouldn't use stuff you don't also have a gyp dependency on

https://codereview.chromium.org/225183018/diff/50001/components/feedback/comm...
File components/feedback/common/feedback_switches.h (right):

https://codereview.chromium.org/225183018/diff/50001/components/feedback/comm...
components/feedback/common/feedback_switches.h:5: #ifndef
COMPONENTS_FEEDBACK_COMMON_SIGNIN_SWITCHES_H_
wrong header guard

https://codereview.chromium.org/225183018/diff/50001/components/feedback/feed...
File components/feedback/feedback_data.h (right):

https://codereview.chromium.org/225183018/diff/50001/components/feedback/feed...
components/feedback/feedback_data.h:24: class FeedbackData : public
base::RefCountedThreadSafe<FeedbackData> {
please put everything within the component into its own namespace

[jochen (gone - plz use gerrit), 2014-04-23 08:22:04]

(maybe some of the other future owners of this component should do a pass over
the code as well?)

[achaulk, 2014-04-23 14:51:32]

https://codereview.chromium.org/225183018/diff/50001/components/feedback.gypi
File components/feedback.gypi (right):

https://codereview.chromium.org/225183018/diff/50001/components/feedback.gypi...
components/feedback.gypi:20: 'feedback/common/feedback_switches.cc',
On 2014/04/23 08:21:24, jochen wrote:
> unless you need to depend on those files from e.g. chrome/common I wouldn't
put
> those into a separate sub directory.

Done.

https://codereview.chromium.org/225183018/diff/50001/components/feedback.gypi...
components/feedback.gypi:33: 'target_name': 'feedback_internal',
It used to be the unittest code wouldn't compile with the other file, but with
the cleaned up deps these should be mergeable

On 2014/04/23 08:21:24, jochen wrote:
> what's the reason to introduce two targets?

https://codereview.chromium.org/225183018/diff/50001/components/feedback/DEPS
File components/feedback/DEPS (right):

https://codereview.chromium.org/225183018/diff/50001/components/feedback/DEPS...
components/feedback/DEPS:8: "+third_party/zlib/google/zip.h",
On 2014/04/23 08:21:24, jochen wrote:
> you shouldn't use stuff you don't also have a gyp dependency on

Done.

https://codereview.chromium.org/225183018/diff/50001/components/feedback/comm...
File components/feedback/common/feedback_switches.h (right):

https://codereview.chromium.org/225183018/diff/50001/components/feedback/comm...
components/feedback/common/feedback_switches.h:5: #ifndef
COMPONENTS_FEEDBACK_COMMON_SIGNIN_SWITCHES_H_
On 2014/04/23 08:21:24, jochen wrote:
> wrong header guard

Done.

https://codereview.chromium.org/225183018/diff/50001/components/feedback/feed...
File components/feedback/feedback_data.h (right):

https://codereview.chromium.org/225183018/diff/50001/components/feedback/feed...
components/feedback/feedback_data.h:24: class FeedbackData : public
base::RefCountedThreadSafe<FeedbackData> {
On 2014/04/23 08:21:24, jochen wrote:
> please put everything within the component into its own namespace

Done.

[achaulk, 2014-04-28 16:58:34]

anyone else have any comments?

[gauravsh, 2014-04-28 20:07:53]

On 2014/04/28 16:58:34, achaulk wrote:
> anyone else have any comments?

rkc: Can you please take a look? This is blocking other work. Thanks!

[rkc, 2014-04-28 22:41:32]

LGTM provided comments are addressed.

https://codereview.chromium.org/225183018/diff/70001/components/components.gyp
File components/components.gyp (right):

https://codereview.chromium.org/225183018/diff/70001/components/components.gy...
components/components.gyp:24: 'feedback.gypi',
Currently Feedback builds for Android builds also, while we don't support Chrome
feedback on Android. Since we're creating this separate Feedback gypi, we should
make the feedback build conditional on OS != "android".

This does not have to be done in this CL, as long as we put a todo here with a
crbug referenced in it to do so in the near future.

https://codereview.chromium.org/225183018/diff/70001/components/feedback/OWNERS
File components/feedback/OWNERS (right):

https://codereview.chromium.org/225183018/diff/70001/components/feedback/OWNE...
components/feedback/OWNERS:3: rkc@chromium.org
Nit: I don't really need to be on this owners list anymore.

https://codereview.chromium.org/225183018/diff/70001/components/feedback/feed...
File components/feedback/feedback_uploader.cc (right):

https://codereview.chromium.org/225183018/diff/70001/components/feedback/feed...
components/feedback/feedback_uploader.cc:50: dispatch_callback_ =
base::Bind(&FeedbackUploader::DispatchReport,
Extract common functionality in the constructors to another Init method, call
the Init from both the constructors. It is only one line right now, but this may
later become more.

https://codereview.chromium.org/225183018/diff/70001/components/feedback/feed...
File components/feedback/feedback_uploader_unittest.cc (right):

https://codereview.chromium.org/225183018/diff/70001/components/feedback/feed...
components/feedback/feedback_uploader_unittest.cc:43: return new
MockUploader(context);
Any particular reason we're using a mock uploader here instead of using the
actual Feedback uploader?

[jochen (gone - plz use gerrit), 2014-04-29 08:00:45]

https://codereview.chromium.org/225183018/diff/70001/components/feedback/DEPS
File components/feedback/DEPS (right):

https://codereview.chromium.org/225183018/diff/70001/components/feedback/DEPS...
components/feedback/DEPS:4: "+content/public/browser",
the gyp target doesn't have dependencies on any of the above three targets?

https://codereview.chromium.org/225183018/diff/70001/components/feedback/DEPS...
components/feedback/DEPS:5: "+content/public/test",
Since you list individual content targets (which is great) you could also add
"-content" at the top

[jochen (gone - plz use gerrit), 2014-04-29 08:01:00]

lgtm with comments addressed

[achaulk, 2014-04-30 17:12:34]

https://codereview.chromium.org/225183018/diff/70001/components/components.gyp
File components/components.gyp (right):

https://codereview.chromium.org/225183018/diff/70001/components/components.gy...
components/components.gyp:24: 'feedback.gypi',
On 2014/04/28 22:41:32, Rahul Chaturvedi wrote:
> Currently Feedback builds for Android builds also, while we don't support
Chrome
> feedback on Android. Since we're creating this separate Feedback gypi, we
should
> make the feedback build conditional on OS != "android".
> 
> This does not have to be done in this CL, as long as we put a todo here with a
> crbug referenced in it to do so in the near future.

Done.

https://codereview.chromium.org/225183018/diff/70001/components/feedback/DEPS
File components/feedback/DEPS (right):

https://codereview.chromium.org/225183018/diff/70001/components/feedback/DEPS...
components/feedback/DEPS:4: "+content/public/browser",
On 2014/04/29 08:00:46, jochen (OOO Wed-Thu) wrote:
> the gyp target doesn't have dependencies on any of the above three targets?

I thought I had added them. Done.

https://codereview.chromium.org/225183018/diff/70001/components/feedback/DEPS...
components/feedback/DEPS:5: "+content/public/test",
On 2014/04/29 08:00:46, jochen (OOO Wed-Thu) wrote:
> Since you list individual content targets (which is great) you could also add
> "-content" at the top

Done.

https://codereview.chromium.org/225183018/diff/70001/components/feedback/OWNERS
File components/feedback/OWNERS (right):

https://codereview.chromium.org/225183018/diff/70001/components/feedback/OWNE...
components/feedback/OWNERS:3: rkc@chromium.org
On 2014/04/28 22:41:32, Rahul Chaturvedi wrote:
> Nit: I don't really need to be on this owners list anymore.

Done.

https://codereview.chromium.org/225183018/diff/70001/components/feedback/feed...
File components/feedback/feedback_uploader.cc (right):

https://codereview.chromium.org/225183018/diff/70001/components/feedback/feed...
components/feedback/feedback_uploader.cc:50: dispatch_callback_ =
base::Bind(&FeedbackUploader::DispatchReport,
On 2014/04/28 22:41:32, Rahul Chaturvedi wrote:
> Extract common functionality in the constructors to another Init method, call
> the Init from both the constructors. It is only one line right now, but this
may
> later become more.

Done.

https://codereview.chromium.org/225183018/diff/70001/components/feedback/feed...
File components/feedback/feedback_uploader_unittest.cc (right):

https://codereview.chromium.org/225183018/diff/70001/components/feedback/feed...
components/feedback/feedback_uploader_unittest.cc:43: return new
MockUploader(context);
On 2014/04/28 22:41:32, Rahul Chaturvedi wrote:
> Any particular reason we're using a mock uploader here instead of using the
> actual Feedback uploader?

Oh, this is a holdover when when the actual uploader code was split off into a
different target. I'll put it back

[achaulk, 2014-04-30 17:13:42]

The CQ bit was checked by achaulk@chromium.org

[commit-bot: I haz the power, 2014-04-30 17:18:52]

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

[commit-bot: I haz the power, 2014-04-30 17:19:25]

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

[commit-bot: I haz the power, 2014-04-30 17:19:26]

Failed to apply patch for chrome/chrome_browser.gypi:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/chrome_browser.gypi
  Hunk #2 succeeded at 34 (offset 1 line).
  Hunk #3 succeeded at 340 (offset -22 lines).
  Hunk #4 FAILED at 375.
  Hunk #5 succeeded at 3188 (offset -18 lines).
  Hunk #6 succeeded at 3489 (offset -7 lines).
  1 out of 6 hunks FAILED -- saving rejects to file
chrome/chrome_browser.gypi.rej

Patch:       chrome/chrome_browser.gypi
Index: chrome/chrome_browser.gypi
diff --git a/chrome/chrome_browser.gypi b/chrome/chrome_browser.gypi
index
038c58b5ee148666f2ebe203d90c2d2e39b16f06..855bdd50a522db35f1f5260a7d2d987ab4a313e8
100644
--- a/chrome/chrome_browser.gypi
+++ b/chrome/chrome_browser.gypi
@@ -21,7 +21,6 @@
         'chrome_resources.gyp:theme_resources',
         'common',
         'common_net',
-        'feedback_proto',
         'in_memory_url_index_cache_proto',
         'probe_message_proto',
         'safe_browsing_proto',
@@ -34,6 +33,7 @@
         '../components/components.gyp:domain_reliability',
         '../components/components.gyp:favicon_base',
         '../components/components.gyp:favicon_core',
+        '../components/components.gyp:feedback_component',
         '../components/components.gyp:infobars_core',
         '../components/components.gyp:navigation_metrics',
         '../components/components.gyp:os_crypt',
@@ -362,22 +362,9 @@
         'browser/browsing_data/browsing_data_cookie_helper.h',
         'browser/browsing_data/cookies_tree_model.cc',
         'browser/browsing_data/cookies_tree_model.h',
-        'browser/feedback/feedback_data.cc',
-        'browser/feedback/feedback_data.h',
         'browser/feedback/feedback_profile_observer.cc',
         'browser/feedback/feedback_profile_observer.h',
-        'browser/feedback/feedback_report.cc',
-        'browser/feedback/feedback_report.h',
-        'browser/feedback/feedback_util.cc',
-        'browser/feedback/feedback_util.h',
-        'browser/feedback/feedback_uploader.cc',
-        'browser/feedback/feedback_uploader.h',
-        'browser/feedback/feedback_uploader_chrome.cc',
-        'browser/feedback/feedback_uploader_chrome.h',
-        'browser/feedback/feedback_uploader_delegate.cc',
-        'browser/feedback/feedback_uploader_delegate.h',
-        'browser/feedback/feedback_uploader_factory.cc',
-        'browser/feedback/feedback_uploader_factory.h',
+        'browser/feedback/show_feedback_page.cc',
         'browser/feedback/system_logs/about_system_logs_fetcher.cc',
         'browser/feedback/system_logs/about_system_logs_fetcher.h',
         'browser/feedback/system_logs/scrubbed_system_logs_fetcher.cc',
@@ -388,8 +375,6 @@
        
'browser/feedback/system_logs/log_sources/chrome_internal_log_source.h',
        
'browser/feedback/system_logs/log_sources/memory_details_log_source.cc',
         'browser/feedback/system_logs/log_sources/memory_details_log_source.h',
-        'browser/feedback/tracing_manager.cc',
-        'browser/feedback/tracing_manager.h',
         'browser/captive_portal/captive_portal_detector.cc',
         'browser/captive_portal/captive_portal_detector.h',
         'browser/captive_portal/captive_portal_login_detector.cc',
@@ -3219,11 +3204,6 @@
             'browser/download/download_shelf.cc',
             'browser/download/download_shelf_context_menu.cc',
             'browser/drive/drive_switches.cc',
-            'browser/feedback/proto/annotations.proto',
-            'browser/feedback/proto/chrome.proto',
-            'browser/feedback/proto/common.proto',
-            'browser/feedback/proto/dom.proto',
-            'browser/feedback/proto/math.proto',
             'browser/gpu/chrome_gpu_util.cc',
             'browser/idle_android.cc',
             'browser/idle.cc',
@@ -3514,25 +3494,6 @@
       'includes': [ '../build/protoc.gypi' ]
     },
     {
-      # Protobuf compiler / generate rule for feedback
-      'target_name': 'feedback_proto',
-      'type': 'static_library',
-      'sources': [
-        'browser/feedback/proto/annotations.proto',
-        'browser/feedback/proto/chrome.proto',
-        'browser/feedback/proto/common.proto',
-        'browser/feedback/proto/dom.proto',
-        'browser/feedback/proto/extension.proto',
-        'browser/feedback/proto/math.proto',
-        'browser/feedback/proto/web.proto',
-      ],
-      'variables': {
-        'proto_in_dir': 'browser/feedback/proto',
-        'proto_out_dir': 'chrome/browser/feedback/proto',
-      },
-      'includes': [ '../build/protoc.gypi' ]
-    },
-    {
       # Protobuf compiler / generator for the safebrowsing reporting
       # protocol buffer.
       'target_name': 'safe_browsing_report_proto',

[achaulk, 2014-04-30 17:24:17]

The CQ bit was checked by achaulk@chromium.org

[commit-bot: I haz the power, 2014-04-30 17:25:10]

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

[commit-bot: I haz the power, 2014-04-30 17:34:25]

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

[commit-bot: I haz the power, 2014-04-30 17:34:26]

Try jobs failed on following builders:
  android_clang_dbg on tryserver.chromium

[achaulk, 2014-04-30 17:42:39]

The CQ bit was checked by achaulk@chromium.org

[commit-bot: I haz the power, 2014-04-30 17:43:29]

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

[commit-bot: I haz the power, 2014-04-30 17:49:00]

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

[commit-bot: I haz the power, 2014-04-30 17:49:00]

Try jobs failed on following builders:
  linux_chromium_gn_rel on tryserver.chromium

[achaulk, 2014-04-30 17:59:23]

The CQ bit was checked by achaulk@chromium.org

[commit-bot: I haz the power, 2014-04-30 18:01:53]

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

[commit-bot: I haz the power, 2014-04-30 18:13:00]

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

[commit-bot: I haz the power, 2014-04-30 18:13:01]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
  linux_chromium_rel on tryserver.chromium

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

The CQ bit was checked by achaulk@chromium.org

[commit-bot: I haz the power, 2014-04-30 18:19:56]

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

[commit-bot: I haz the power, 2014-04-30 18:24:29]

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

[commit-bot: I haz the power, 2014-04-30 18:24:29]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium

[achaulk, 2014-05-01 15:59:07]

The CQ bit was checked by achaulk@chromium.org

[commit-bot: I haz the power, 2014-05-01 16:00:00]

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

[commit-bot: I haz the power, 2014-05-01 16:07:29]

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

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

Try jobs failed on following builders:
  ios_dbg_simulator on tryserver.chromium
  ios_rel_device on tryserver.chromium
  ios_rel_device_ninja on tryserver.chromium

[achaulk, 2014-05-01 17:18:58]

The CQ bit was checked by achaulk@chromium.org

[commit-bot: I haz the power, 2014-05-01 17:19:43]

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

[commit-bot: I haz the power, 2014-05-01 17:38:48]

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

[commit-bot: I haz the power, 2014-05-01 17:38:49]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium

[achaulk, 2014-05-01 17:56:23]

Oh right DEPS owners lgtms
@gavinp: for net/
@hshi: for third_party/zlib/zip

[hshi1, 2014-05-05 17:24:09]

On 2014/05/01 17:56:23, achaulk wrote:
> Oh right DEPS owners lgtms
> @gavinp: for net/
> @hshi: for third_party/zlib/zip

DEPS for third_party/zlib/zip LGTM

[gauravsh, 2014-05-07 00:28:22]

+cbentzel/mmenke who are src/net OWNERS

We need an OWNERS review for adding the net/ to feedback component's DEPS. Could
you help?

Here's the relevant file:
https://codereview.chromium.org/225183018/patch/190001/200052

[gavinp, 2014-05-07 14:23:00]

On 2014/05/07 00:28:22, gauravsh wrote:
> +cbentzel/mmenke who are src/net OWNERS
> 
> We need an OWNERS review for adding the net/ to feedback component's DEPS.
Could
> you help?
> 
> Here's the relevant file:
> https://codereview.chromium.org/225183018/patch/190001/200052

LGTM.

[gavinp, 2014-05-07 14:24:17]

On 2014/05/07 14:23:00, gavinp wrote:
> On 2014/05/07 00:28:22, gauravsh wrote:
> > +cbentzel/mmenke who are src/net OWNERS
> > 
> > We need an OWNERS review for adding the net/ to feedback component's DEPS.
> Could
> > you help?
> > 
> > Here's the relevant file:
> > https://codereview.chromium.org/225183018/patch/190001/200052
> 
> LGTM.

Also, I'm a third_party/zlib owner, and that code LGTM too.

[achaulk, 2014-05-07 14:24:19]

The CQ bit was checked by achaulk@chromium.org

[commit-bot: I haz the power, 2014-05-07 14:27:34]

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

[commit-bot: I haz the power, 2014-05-07 14:52:05]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
  chromium_presubmit on tryserver.chromium
  linux_chromium_chromeos_rel on tryserver.chromium
  win_chromium_rel on tryserver.chromium

[commit-bot: I haz the power, 2014-05-07 15:06:29]

FYI, CQ is re-trying this CL (attempt #2).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
  chromium_presubmit on tryserver.chromium
  linux_chromium_chromeos_rel on tryserver.chromium
  win_chromium_rel on tryserver.chromium

[commit-bot: I haz the power, 2014-05-07 15:12:54]

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

[commit-bot: I haz the power, 2014-05-07 15:12:55]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
  linux_chromium_chromeos_rel on tryserver.chromium

[achaulk, 2014-05-07 16:34:05]

The CQ bit was checked by achaulk@chromium.org

[commit-bot: I haz the power, 2014-05-07 16:39:22]

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

[achaulk, 2014-05-07 21:17:56]

The CQ bit was checked by achaulk@chromium.org

[commit-bot: I haz the power, 2014-05-07 21:20:21]

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

[commit-bot: I haz the power, 2014-05-07 23:09:09]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium

[commit-bot: I haz the power, 2014-05-08 00:33:23]

FYI, CQ is re-trying this CL (attempt #2).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium

[commit-bot: I haz the power, 2014-05-08 02:28:55]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium

[commit-bot: I haz the power, 2014-05-08 02:51:32]

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

[commit-bot: I haz the power, 2014-05-08 02:51:33]

Try jobs failed on following builders:
  android_dbg_triggered_tests on tryserver.chromium

[achaulk, 2014-05-08 14:14:57]

Should be no way this CL is causing the test failures

[achaulk, 2014-05-08 14:15:11]

The CQ bit was checked by achaulk@chromium.org

[commit-bot: I haz the power, 2014-05-08 14:16:03]

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

[commit-bot: I haz the power, 2014-05-09 02:17:24]

Change committed as 269152

[tfarina, 2014-05-16 18:45:33]

https://codereview.chromium.org/225183018/diff/210001/components/feedback/fee...
File components/feedback/feedback_uploader_chrome.h (right):

https://codereview.chromium.org/225183018/diff/210001/components/feedback/fee...
components/feedback/feedback_uploader_chrome.h:17: class FeedbackUploaderChrome
: public FeedbackUploader,
Why this is was not kept in chrome/?

I think components/ should not know about chrome, since they are in layer lower
than chrome.