Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(30)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 12039079: content: convert user action notification to observer usage (Closed)

Created:
7 years, 11 months ago by Paweł Hajdan Jr.
Modified:
7 years, 10 months ago
Reviewers:
jam
CC:
chromium-reviews, MAD, jar (doing other things), jam, joi+watch-content_chromium.org, darin-cc_chromium.org, Ilya Sherman
Visibility:
Public.

Description

content: convert user action notification to callbacks BUG=170921 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=180392

Patch Set 1 #

Total comments: 1

Patch Set 2 : callbacks #

Total comments: 6

Patch Set 3 : fixes #

Total comments: 4

Patch Set 4 : rebase & fix #

Patch Set 5 : return #

Patch Set 6 : rebase #

Unified diffs Side-by-side diffs Delta from patch set Stats (+101 lines, -93 lines) Patch
M chrome/browser/extensions/api/metrics/metrics_apitest.cc View 1 2 3 chunks +19 lines, -16 lines 0 comments Download
M chrome/browser/metrics/metrics_service.h View 1 2 3 4 chunks +6 lines, -3 lines 0 comments Download
M chrome/browser/metrics/metrics_service.cc View 1 2 3 6 chunks +15 lines, -12 lines 0 comments Download
M chrome/browser/ui/webui/user_actions/user_actions_ui_handler.h View 1 2 1 chunk +5 lines, -12 lines 0 comments Download
M chrome/browser/ui/webui/user_actions/user_actions_ui_handler.cc View 1 2 1 chunk +10 lines, -17 lines 0 comments Download
M content/browser/notification_service_impl_unittest.cc View 10 chunks +17 lines, -14 lines 0 comments Download
M content/browser/user_metrics.cc View 1 2 3 4 3 chunks +21 lines, -6 lines 0 comments Download
M content/public/browser/notification_types.h View 1 2 3 1 chunk +0 lines, -13 lines 0 comments Download
M content/public/browser/user_metrics.h View 1 2 3 2 chunks +8 lines, -0 lines 0 comments Download

Messages

Total messages: 8 (0 generated)
Paweł Hajdan Jr.
7 years, 11 months ago (2013-01-25 00:42:10 UTC) #1
jam
https://codereview.chromium.org/12039079/diff/1/content/public/browser/user_metrics.h File content/public/browser/user_metrics.h (right): https://codereview.chromium.org/12039079/diff/1/content/public/browser/user_metrics.h#newcode58 content/public/browser/user_metrics.h:58: virtual void UserAction(const std::string& action); since this is just ...
7 years, 10 months ago (2013-01-29 23:53:25 UTC) #2
Paweł Hajdan Jr.
PTAL I've checked and some of the cases get triggered from content, so this seems ...
7 years, 10 months ago (2013-01-30 14:56:36 UTC) #3
jam
https://codereview.chromium.org/12039079/diff/6001/chrome/browser/ui/webui/user_actions/user_actions_ui_handler.cc File chrome/browser/ui/webui/user_actions/user_actions_ui_handler.cc (right): https://codereview.chromium.org/12039079/diff/6001/chrome/browser/ui/webui/user_actions/user_actions_ui_handler.cc#newcode18 chrome/browser/ui/webui/user_actions/user_actions_ui_handler.cc:18: base::Unretained(this))); same comment as the other cl about this ...
7 years, 10 months ago (2013-01-30 17:53:23 UTC) #4
Paweł Hajdan Jr.
PTAL https://codereview.chromium.org/12039079/diff/6001/chrome/browser/ui/webui/user_actions/user_actions_ui_handler.h File chrome/browser/ui/webui/user_actions/user_actions_ui_handler.h (right): https://codereview.chromium.org/12039079/diff/6001/chrome/browser/ui/webui/user_actions/user_actions_ui_handler.h#newcode19 chrome/browser/ui/webui/user_actions/user_actions_ui_handler.h:19: void OnUserAction(const std::string& action); On 2013/01/30 17:53:24, jam ...
7 years, 10 months ago (2013-01-31 14:34:44 UTC) #5
jam
lgtm with nit https://codereview.chromium.org/12039079/diff/13001/content/browser/user_metrics.cc File content/browser/user_metrics.cc (right): https://codereview.chromium.org/12039079/diff/13001/content/browser/user_metrics.cc#newcode54 content/browser/user_metrics.cc:54: for (std::vector<ActionCallback>::iterator i = nit: size_t ...
7 years, 10 months ago (2013-01-31 18:02:29 UTC) #6
jam
https://codereview.chromium.org/12039079/diff/13001/content/public/browser/user_metrics.h File content/public/browser/user_metrics.h (right): https://codereview.chromium.org/12039079/diff/13001/content/public/browser/user_metrics.h#newcode11 content/public/browser/user_metrics.h:11: #include "base/threading/non_thread_safe.h" nit: not needed
7 years, 10 months ago (2013-01-31 21:41:54 UTC) #7
Paweł Hajdan Jr.
7 years, 10 months ago (2013-02-01 10:43:57 UTC) #8 
Thank you for review!

https://codereview.chromium.org/12039079/diff/13001/content/browser/user_metr...
File content/browser/user_metrics.cc (right):

https://codereview.chromium.org/12039079/diff/13001/content/browser/user_metr...
content/browser/user_metrics.cc:54: for (std::vector<ActionCallback>::iterator i
=
On 2013/01/31 18:02:29, jam wrote:
> nit: size_t here too

Done.

https://codereview.chromium.org/12039079/diff/13001/content/public/browser/us...
File content/public/browser/user_metrics.h (right):

https://codereview.chromium.org/12039079/diff/13001/content/public/browser/us...
content/public/browser/user_metrics.h:11: #include
"base/threading/non_thread_safe.h"
On 2013/01/31 21:41:55, jam wrote:
> nit: not needed

Done.

Powered by Google App Engine
This is Rietveld 408576698