Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
864253002:
Dump property values for the touch log source (Closed)
|
Created:
5 years, 11 months ago by Shecky Lin Modified:
5 years, 10 months ago CC:
chromium-reviews, ozone-reviews_chromium.org, nkostylev+watch_chromium.org, tdresser+watch_chromium.org, jdduke+watch_chromium.org, oshima+watch_chromium.org, kalyank, stevenjb+watch_chromium.org, davemoore+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDump property values for the touch log source
The CL dumps gesture property values for touch devices that run
with the CrOS gesture library. The format is designed to be
backward-compatible with the X11 behaviour.
Contributed by sheckylin@chromium.org
BUG=450159
TEST=samus ChromeOS build
Committed: https://crrev.com/8b1fb4fdf2b13b71e969cd541e500549163f742b
Cr-Commit-Position: refs/heads/master@{#313446}
Committed: https://crrev.com/e09a9b07f189a5ae9cd5f01cba6be9b97e76cad1
Cr-Commit-Position: refs/heads/master@{#313555}
Patch Set 1 #Patch Set 2 : Sort property values. #
Total comments: 8
Patch Set 3 : Fixed nits. #
Total comments: 1
Patch Set 4 : Change to use asynchronous callbacks #Patch Set 5 : Use base::WorkerPool to post task in input_controller_evdev #
Total comments: 10
Patch Set 6 : Cleaned up callback hops #
Total comments: 2
Patch Set 7 : run task on the UI thread instead #Patch Set 8 : update comments #Patch Set 9 : fix ifdefs #
Total comments: 2
Patch Set 10 : fixed ifdefs again #Patch Set 11 : fixed typo #Patch Set 12 : fix %lu -> %zu #
Messages
Total messages: 38 (6 generated)
sheckylin@chromium.org changed reviewers: + achuith@chromium.org, adlr@chromium.org, alexst@chromium.org, spang@chromium.org
Hey guys, This is the 1st CL to implement the touch log collection. Please take a look of it. Thanks!
lgtm to me. please wait for others. https://codereview.chromium.org/864253002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/system_logs/touch_log_source_ozone.cc (right): https://codereview.chromium.org/864253002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/system_logs/touch_log_source_ozone.cc:35: (*response)["hack-33025-touchpad"] = ui::OzonePlatform::GetInstance() Make this a const char[] as well, and add a comment explaining what hack-33025 means? https://codereview.chromium.org/864253002/diff/20001/ui/events/ozone/evdev/in... File ui/events/ozone/evdev/input_controller_evdev.cc (right): https://codereview.chromium.org/864253002/diff/20001/ui/events/ozone/evdev/in... ui/events/ozone/evdev/input_controller_evdev.cc:45: Add a function comment https://codereview.chromium.org/864253002/diff/20001/ui/events/ozone/evdev/in... ui/events/ozone/evdev/input_controller_evdev.cc:57: std::string DumpGesturePropertyValue(GesturesProp* property) { Add a function comment https://codereview.chromium.org/864253002/diff/20001/ui/events/ozone/evdev/li... File ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.cc (right): https://codereview.chromium.org/864253002/diff/20001/ui/events/ozone/evdev/li... ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.cc:889: internal::ScopedPropertiesMap::const_iterator it = Move this to the initializer of the for loop below.
Done with achuith's comments. Thanks! spang/alexst: could you take a look of the ozone part of the patch? https://codereview.chromium.org/864253002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/system_logs/touch_log_source_ozone.cc (right): https://codereview.chromium.org/864253002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/system_logs/touch_log_source_ozone.cc:35: (*response)["hack-33025-touchpad"] = ui::OzonePlatform::GetInstance() On 2015/01/22 20:42:12, achuithb wrote: > Make this a const char[] as well, and add a comment explaining what hack-33025 > means? Done. https://codereview.chromium.org/864253002/diff/20001/ui/events/ozone/evdev/in... File ui/events/ozone/evdev/input_controller_evdev.cc (right): https://codereview.chromium.org/864253002/diff/20001/ui/events/ozone/evdev/in... ui/events/ozone/evdev/input_controller_evdev.cc:45: On 2015/01/22 20:42:12, achuithb wrote: > Add a function comment Done. https://codereview.chromium.org/864253002/diff/20001/ui/events/ozone/evdev/in... ui/events/ozone/evdev/input_controller_evdev.cc:57: std::string DumpGesturePropertyValue(GesturesProp* property) { On 2015/01/22 20:42:12, achuithb wrote: > Add a function comment Done. https://codereview.chromium.org/864253002/diff/20001/ui/events/ozone/evdev/li... File ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.cc (right): https://codereview.chromium.org/864253002/diff/20001/ui/events/ozone/evdev/li... ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.cc:889: internal::ScopedPropertiesMap::const_iterator it = On 2015/01/22 20:42:12, achuithb wrote: > Move this to the initializer of the for loop below. Done.
https://codereview.chromium.org/864253002/diff/40001/ui/ozone/public/input_co... File ui/ozone/public/input_controller.h (right): https://codereview.chromium.org/864253002/diff/40001/ui/ozone/public/input_co... ui/ozone/public/input_controller.h:60: virtual std::string GetGestureDeviceStatus() = 0; This needs to be asynchronous because we are moving gestures to a different thread to avoid cursor jank. I do not want to land new code that requires synchronous access to gestures. Also, please don't say "gestures" in the name. Functions in this interface should have generic names. So something like: typedef base::Callback<void(scoped_ptr<std::string>)> GetTouchpadDebugLogsReply; void GetTouchpadDebugLogs(const GetTouchpadDebugLogsReply& reply); How big can the log get? If it is very large, it might be necessary to stream the data instead of passing it in one big string.
On 2015/01/23 18:39:43, spang wrote: > https://codereview.chromium.org/864253002/diff/40001/ui/ozone/public/input_co... > File ui/ozone/public/input_controller.h (right): > > https://codereview.chromium.org/864253002/diff/40001/ui/ozone/public/input_co... > ui/ozone/public/input_controller.h:60: virtual std::string > GetGestureDeviceStatus() = 0; > This needs to be asynchronous because we are moving gestures to a different > thread to avoid cursor jank. I do not want to land new code that requires > synchronous access to gestures. > > Also, please don't say "gestures" in the name. Functions in this interface > should have generic names. > > So something like: > > typedef base::Callback<void(scoped_ptr<std::string>)> > GetTouchpadDebugLogsReply; > void GetTouchpadDebugLogs(const GetTouchpadDebugLogsReply& reply); Got it. I am checking your thread separation patches to see how I can fit mine into this new setup. Do you have an estimate on how long it would take you to land the changes though? It seems like I couldn't make a patch that works for both cases (before and after separation). I'm asking because we have an increasing need on the log collection to debug touch problems for new devices. > > How big can the log get? If it is very large, it might be necessary to stream > the data instead of passing it in one big string. The log is about 15K bytes for a standard setup (1 touchpad + 1 mouse).
On 2015/01/24 05:58:55, Shecky Lin wrote: > On 2015/01/23 18:39:43, spang wrote: > > > https://codereview.chromium.org/864253002/diff/40001/ui/ozone/public/input_co... > > File ui/ozone/public/input_controller.h (right): > > > > > https://codereview.chromium.org/864253002/diff/40001/ui/ozone/public/input_co... > > ui/ozone/public/input_controller.h:60: virtual std::string > > GetGestureDeviceStatus() = 0; > > This needs to be asynchronous because we are moving gestures to a different > > thread to avoid cursor jank. I do not want to land new code that requires > > synchronous access to gestures. > > > > Also, please don't say "gestures" in the name. Functions in this interface > > should have generic names. > > > > So something like: > > > > typedef base::Callback<void(scoped_ptr<std::string>)> > > GetTouchpadDebugLogsReply; > > void GetTouchpadDebugLogs(const GetTouchpadDebugLogsReply& reply); > > Got it. I am checking your thread separation patches to see how I can fit mine > into this new setup. > > Do you have an estimate on how long it would take you to land the changes > though? It seems like I couldn't make a patch that works for both cases (before > and after separation). I'm asking because we have an increasing need on the log > collection to debug touch problems for new devices. It seems like I can actually implement the whole log collection thing asynchronously and then migrate to your new InputDeviceFactoryEvdev design later. So your work could be carried out in parallel to mine. Thanks for the hard work on reducing the cursor jank by the way! > > > > > How big can the log get? If it is very large, it might be necessary to stream > > the data instead of passing it in one big string. > > The log is about 15K bytes for a standard setup (1 touchpad + 1 mouse).
On 2015/01/24 07:41:44, Shecky Lin wrote: > On 2015/01/24 05:58:55, Shecky Lin wrote: > > On 2015/01/23 18:39:43, spang wrote: > > > > > > https://codereview.chromium.org/864253002/diff/40001/ui/ozone/public/input_co... > > > File ui/ozone/public/input_controller.h (right): > > > > > > > > > https://codereview.chromium.org/864253002/diff/40001/ui/ozone/public/input_co... > > > ui/ozone/public/input_controller.h:60: virtual std::string > > > GetGestureDeviceStatus() = 0; > > > This needs to be asynchronous because we are moving gestures to a different > > > thread to avoid cursor jank. I do not want to land new code that requires > > > synchronous access to gestures. > > > > > > Also, please don't say "gestures" in the name. Functions in this interface > > > should have generic names. > > > > > > So something like: > > > > > > typedef base::Callback<void(scoped_ptr<std::string>)> > > > GetTouchpadDebugLogsReply; > > > void GetTouchpadDebugLogs(const GetTouchpadDebugLogsReply& reply); > > > > Got it. I am checking your thread separation patches to see how I can fit mine > > into this new setup. > > > > Do you have an estimate on how long it would take you to land the changes > > though? It seems like I couldn't make a patch that works for both cases > (before > > and after separation). I'm asking because we have an increasing need on the > log > > collection to debug touch problems for new devices. > > It seems like I can actually implement the whole log collection thing > asynchronously and then migrate to your new InputDeviceFactoryEvdev design > later. So your work could be carried out in parallel to mine. Thanks for the > hard work on reducing the cursor jank by the way! We can land this patch first as long as it is asynchronous. > > > > > > > > > How big can the log get? If it is very large, it might be necessary to > stream > > > the data instead of passing it in one big string. > > > > The log is about 15K bytes for a standard setup (1 touchpad + 1 mouse).
I have updated the patch to make it asynchronous. However, I ran into the
following presumbit error:
** Presubmit ERRORS **
You added one or more #includes that violate checkdeps rules.
ui/events/ozone/evdev/input_controller_evdev.cc
Illegal include: "content/public/browser/browser_thread.h"
Because of no rule applying.
Do you have any idea how I can fix this? Thanks!
On 2015/01/26 18:04:37, spang wrote:
> On 2015/01/24 07:41:44, Shecky Lin wrote:
> > On 2015/01/24 05:58:55, Shecky Lin wrote:
> > > On 2015/01/23 18:39:43, spang wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/864253002/diff/40001/ui/ozone/public/input_co...
> > > > File ui/ozone/public/input_controller.h (right):
> > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/864253002/diff/40001/ui/ozone/public/input_co...
> > > > ui/ozone/public/input_controller.h:60: virtual std::string
> > > > GetGestureDeviceStatus() = 0;
> > > > This needs to be asynchronous because we are moving gestures to a
> different
> > > > thread to avoid cursor jank. I do not want to land new code that
requires
> > > > synchronous access to gestures.
> > > >
> > > > Also, please don't say "gestures" in the name. Functions in this
interface
> > > > should have generic names.
> > > >
> > > > So something like:
> > > >
> > > > typedef base::Callback<void(scoped_ptr<std::string>)>
> > > > GetTouchpadDebugLogsReply;
> > > > void GetTouchpadDebugLogs(const GetTouchpadDebugLogsReply& reply);
> > >
> > > Got it. I am checking your thread separation patches to see how I can fit
> mine
> > > into this new setup.
> > >
> > > Do you have an estimate on how long it would take you to land the changes
> > > though? It seems like I couldn't make a patch that works for both cases
> > (before
> > > and after separation). I'm asking because we have an increasing need on
the
> > log
> > > collection to debug touch problems for new devices.
> >
> > It seems like I can actually implement the whole log collection thing
> > asynchronously and then migrate to your new InputDeviceFactoryEvdev design
> > later. So your work could be carried out in parallel to mine. Thanks for the
> > hard work on reducing the cursor jank by the way!
>
> We can land this patch first as long as it is asynchronous.
>
> >
> > >
> > > >
> > > > How big can the log get? If it is very large, it might be necessary to
> > stream
> > > > the data instead of passing it in one big string.
> > >
> > > The log is about 15K bytes for a standard setup (1 touchpad + 1 mouse).
This usually points to a structural problem. I believe you can't include something from content in ui/: https://code.google.com/p/chromium/codesearch#chromium/src/ui/DEPS On 2015/01/27 01:29:14, Shecky Lin wrote: > I have updated the patch to make it asynchronous. However, I ran into the > following presumbit error: > > ** Presubmit ERRORS ** > You added one or more #includes that violate checkdeps rules. > ui/events/ozone/evdev/input_controller_evdev.cc > Illegal include: "content/public/browser/browser_thread.h" > Because of no rule applying. > > Do you have any idea how I can fix this? Thanks! > > On 2015/01/26 18:04:37, spang wrote: > > On 2015/01/24 07:41:44, Shecky Lin wrote: > > > On 2015/01/24 05:58:55, Shecky Lin wrote: > > > > On 2015/01/23 18:39:43, spang wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/864253002/diff/40001/ui/ozone/public/input_co... > > > > > File ui/ozone/public/input_controller.h (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/864253002/diff/40001/ui/ozone/public/input_co... > > > > > ui/ozone/public/input_controller.h:60: virtual std::string > > > > > GetGestureDeviceStatus() = 0; > > > > > This needs to be asynchronous because we are moving gestures to a > > different > > > > > thread to avoid cursor jank. I do not want to land new code that > requires > > > > > synchronous access to gestures. > > > > > > > > > > Also, please don't say "gestures" in the name. Functions in this > interface > > > > > should have generic names. > > > > > > > > > > So something like: > > > > > > > > > > typedef base::Callback<void(scoped_ptr<std::string>)> > > > > > GetTouchpadDebugLogsReply; > > > > > void GetTouchpadDebugLogs(const GetTouchpadDebugLogsReply& reply); > > > > > > > > Got it. I am checking your thread separation patches to see how I can fit > > mine > > > > into this new setup. > > > > > > > > Do you have an estimate on how long it would take you to land the changes > > > > though? It seems like I couldn't make a patch that works for both cases > > > (before > > > > and after separation). I'm asking because we have an increasing need on > the > > > log > > > > collection to debug touch problems for new devices. > > > > > > It seems like I can actually implement the whole log collection thing > > > asynchronously and then migrate to your new InputDeviceFactoryEvdev design > > > later. So your work could be carried out in parallel to mine. Thanks for the > > > hard work on reducing the cursor jank by the way! > > > > We can land this patch first as long as it is asynchronous. > > > > > > > > > > > > > > > > > > > How big can the log get? If it is very large, it might be necessary to > > > stream > > > > > the data instead of passing it in one big string. > > > > > > > > The log is about 15K bytes for a standard setup (1 touchpad + 1 mouse).
Yeah, looks like the best we can get over there to post tasks is base::WorkerPool. Please take another look of the patch. Thanks guys! On 2015/01/27 01:32:48, achuithb wrote: > This usually points to a structural problem. I believe you can't include > something from content in ui/: > https://code.google.com/p/chromium/codesearch#chromium/src/ui/DEPS > > On 2015/01/27 01:29:14, Shecky Lin wrote: > > I have updated the patch to make it asynchronous. However, I ran into the > > following presumbit error: > > > > ** Presubmit ERRORS ** > > You added one or more #includes that violate checkdeps rules. > > ui/events/ozone/evdev/input_controller_evdev.cc > > Illegal include: "content/public/browser/browser_thread.h" > > Because of no rule applying. > > > > Do you have any idea how I can fix this? Thanks! > > > > On 2015/01/26 18:04:37, spang wrote: > > > On 2015/01/24 07:41:44, Shecky Lin wrote: > > > > On 2015/01/24 05:58:55, Shecky Lin wrote: > > > > > On 2015/01/23 18:39:43, spang wrote: > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/864253002/diff/40001/ui/ozone/public/input_co... > > > > > > File ui/ozone/public/input_controller.h (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/864253002/diff/40001/ui/ozone/public/input_co... > > > > > > ui/ozone/public/input_controller.h:60: virtual std::string > > > > > > GetGestureDeviceStatus() = 0; > > > > > > This needs to be asynchronous because we are moving gestures to a > > > different > > > > > > thread to avoid cursor jank. I do not want to land new code that > > requires > > > > > > synchronous access to gestures. > > > > > > > > > > > > Also, please don't say "gestures" in the name. Functions in this > > interface > > > > > > should have generic names. > > > > > > > > > > > > So something like: > > > > > > > > > > > > typedef base::Callback<void(scoped_ptr<std::string>)> > > > > > > GetTouchpadDebugLogsReply; > > > > > > void GetTouchpadDebugLogs(const GetTouchpadDebugLogsReply& reply); > > > > > > > > > > Got it. I am checking your thread separation patches to see how I can > fit > > > mine > > > > > into this new setup. > > > > > > > > > > Do you have an estimate on how long it would take you to land the > changes > > > > > though? It seems like I couldn't make a patch that works for both cases > > > > (before > > > > > and after separation). I'm asking because we have an increasing need on > > the > > > > log > > > > > collection to debug touch problems for new devices. > > > > > > > > It seems like I can actually implement the whole log collection thing > > > > asynchronously and then migrate to your new InputDeviceFactoryEvdev design > > > > later. So your work could be carried out in parallel to mine. Thanks for > the > > > > hard work on reducing the cursor jank by the way! > > > > > > We can land this patch first as long as it is asynchronous. > > > > > > > > > > > > > > > > > > > > > > > > How big can the log get? If it is very large, it might be necessary to > > > > stream > > > > > > the data instead of passing it in one big string. > > > > > > > > > > The log is about 15K bytes for a standard setup (1 touchpad + 1 mouse).
https://codereview.chromium.org/864253002/diff/80001/chrome/browser/chromeos/... File chrome/browser/chromeos/system_logs/touch_log_source_ozone.cc (left): https://codereview.chromium.org/864253002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/system_logs/touch_log_source_ozone.cc:24: ash::TouchHudDebug::GetAllAsDictionary(); This seems scary.. what makes it safe to call ash from the blocking pool? https://codereview.chromium.org/864253002/diff/80001/chrome/browser/chromeos/... File chrome/browser/chromeos/system_logs/touch_log_source_ozone.cc (right): https://codereview.chromium.org/864253002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/system_logs/touch_log_source_ozone.cc:78: base::Bind(&CollectStatusLog, response, callback)); Do you need this task hop? Aren't we already on UI? If so you can inline CollectStatusLog() here. https://codereview.chromium.org/864253002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/system_logs/touch_log_source_ozone.cc:91: ->PostTask(FROM_HERE, base::Bind(&StartLogRetrieval, response, callback)); Use base::Passed() to pass ownership of response through the callback. This makes the code a lot clearer and also prevents memory leaks if PostTask fails and the the callback never executes. https://codereview.chromium.org/864253002/diff/80001/ui/ozone/public/input_co... File ui/ozone/public/input_controller.cc (right): https://codereview.chromium.org/864253002/diff/80001/ui/ozone/public/input_co... ui/ozone/public/input_controller.cc:130: NOTIMPLEMENTED(); I think you should call the reply to avoid waiting forever.
On 2015/01/27 20:15:13, spang wrote: > https://codereview.chromium.org/864253002/diff/80001/chrome/browser/chromeos/... > File chrome/browser/chromeos/system_logs/touch_log_source_ozone.cc (left): > > https://codereview.chromium.org/864253002/diff/80001/chrome/browser/chromeos/... > chrome/browser/chromeos/system_logs/touch_log_source_ozone.cc:24: > ash::TouchHudDebug::GetAllAsDictionary(); > This seems scary.. what makes it safe to call ash from the blocking pool? Sorry, but I am unsure what they are for. This thing was originally in the X11 counterpart and I just duped them when I make the ozone one. I will cc the original author of it (miletus@) to see if he has anything to say about it. > > https://codereview.chromium.org/864253002/diff/80001/chrome/browser/chromeos/... > File chrome/browser/chromeos/system_logs/touch_log_source_ozone.cc (right): > > https://codereview.chromium.org/864253002/diff/80001/chrome/browser/chromeos/... > chrome/browser/chromeos/system_logs/touch_log_source_ozone.cc:78: > base::Bind(&CollectStatusLog, response, callback)); > Do you need this task hop? Aren't we already on UI? > > If so you can inline CollectStatusLog() here. I've tried to inline the call before and it would just crash. I thought blocking pool tasks aren't necessarily run on the UI thread? > > https://codereview.chromium.org/864253002/diff/80001/chrome/browser/chromeos/... > chrome/browser/chromeos/system_logs/touch_log_source_ozone.cc:91: > ->PostTask(FROM_HERE, base::Bind(&StartLogRetrieval, response, callback)); > Use base::Passed() to pass ownership of response through the callback. This > makes the code a lot clearer and also prevents memory leaks if PostTask fails > and the the callback never executes. > > https://codereview.chromium.org/864253002/diff/80001/ui/ozone/public/input_co... > File ui/ozone/public/input_controller.cc (right): > > https://codereview.chromium.org/864253002/diff/80001/ui/ozone/public/input_co... > ui/ozone/public/input_controller.cc:130: NOTIMPLEMENTED(); > I think you should call the reply to avoid waiting forever.
sheckylin@chromium.org changed reviewers: + miletus@chromium.org
Please respond to each comment individually using rietveld so it shows up in the right place. On 2015/01/27 20:31:44, Shecky Lin wrote: > On 2015/01/27 20:15:13, spang wrote: > > > https://codereview.chromium.org/864253002/diff/80001/chrome/browser/chromeos/... > > File chrome/browser/chromeos/system_logs/touch_log_source_ozone.cc (left): > > > > > https://codereview.chromium.org/864253002/diff/80001/chrome/browser/chromeos/... > > chrome/browser/chromeos/system_logs/touch_log_source_ozone.cc:24: > > ash::TouchHudDebug::GetAllAsDictionary(); > > This seems scary.. what makes it safe to call ash from the blocking pool? > Sorry, but I am unsure what they are for. This thing was originally in the X11 > counterpart and I just duped them when I make the ozone one. I will cc the > original author of it (miletus@) to see if he has anything to say about it. Yeah I see it was there before. I am still worried though. > > > > > > https://codereview.chromium.org/864253002/diff/80001/chrome/browser/chromeos/... > > File chrome/browser/chromeos/system_logs/touch_log_source_ozone.cc (right): > > > > > https://codereview.chromium.org/864253002/diff/80001/chrome/browser/chromeos/... > > chrome/browser/chromeos/system_logs/touch_log_source_ozone.cc:78: > > base::Bind(&CollectStatusLog, response, callback)); > > Do you need this task hop? Aren't we already on UI? > > > > If so you can inline CollectStatusLog() here. > > I've tried to inline the call before and it would just crash. I thought blocking > pool tasks aren't necessarily run on the UI thread? That's right, sorry I misread. > > > > > > https://codereview.chromium.org/864253002/diff/80001/chrome/browser/chromeos/... > > chrome/browser/chromeos/system_logs/touch_log_source_ozone.cc:91: > > ->PostTask(FROM_HERE, base::Bind(&StartLogRetrieval, response, callback)); > > Use base::Passed() to pass ownership of response through the callback. This > > makes the code a lot clearer and also prevents memory leaks if PostTask fails > > and the the callback never executes. > > > > > https://codereview.chromium.org/864253002/diff/80001/ui/ozone/public/input_co... > > File ui/ozone/public/input_controller.cc (right): > > > > > https://codereview.chromium.org/864253002/diff/80001/ui/ozone/public/input_co... > > ui/ozone/public/input_controller.cc:130: NOTIMPLEMENTED(); > > I think you should call the reply to avoid waiting forever.
https://codereview.chromium.org/864253002/diff/80001/chrome/browser/chromeos/... File chrome/browser/chromeos/system_logs/touch_log_source_ozone.cc (right): https://codereview.chromium.org/864253002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/system_logs/touch_log_source_ozone.cc:78: base::Bind(&CollectStatusLog, response, callback)); On 2015/01/27 20:15:12, spang wrote: > Do you need this task hop? Aren't we already on UI? > > If so you can inline CollectStatusLog() here. My bad, I misread the code, TouchLogSource::Fetch is the UI part. This is right.
https://codereview.chromium.org/864253002/diff/80001/chrome/browser/chromeos/... File chrome/browser/chromeos/system_logs/touch_log_source_ozone.cc (right): https://codereview.chromium.org/864253002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/system_logs/touch_log_source_ozone.cc:78: base::Bind(&CollectStatusLog, response, callback)); On 2015/01/27 20:15:12, spang wrote: > Do you need this task hop? Aren't we already on UI? > > If so you can inline CollectStatusLog() here. My bad, I misread the code, TouchLogSource::Fetch is the UI part. This is right.
https://codereview.chromium.org/864253002/diff/80001/chrome/browser/chromeos/... File chrome/browser/chromeos/system_logs/touch_log_source_ozone.cc (left): https://codereview.chromium.org/864253002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/system_logs/touch_log_source_ozone.cc:24: ash::TouchHudDebug::GetAllAsDictionary(); On 2015/01/27 20:15:13, spang wrote: > This seems scary.. what makes it safe to call ash from the blocking pool? Can you do this part on UI and pass the results along? That would eliminate my concern.
https://codereview.chromium.org/864253002/diff/80001/chrome/browser/chromeos/... File chrome/browser/chromeos/system_logs/touch_log_source_ozone.cc (left): https://codereview.chromium.org/864253002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/system_logs/touch_log_source_ozone.cc:24: ash::TouchHudDebug::GetAllAsDictionary(); On 2015/01/27 20:44:56, spang wrote: > On 2015/01/27 20:15:13, spang wrote: > > This seems scary.. what makes it safe to call ash from the blocking pool? > > Can you do this part on UI and pass the results along? That would eliminate my > concern. Done. https://codereview.chromium.org/864253002/diff/80001/chrome/browser/chromeos/... File chrome/browser/chromeos/system_logs/touch_log_source_ozone.cc (right): https://codereview.chromium.org/864253002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/system_logs/touch_log_source_ozone.cc:78: base::Bind(&CollectStatusLog, response, callback)); On 2015/01/27 20:42:37, spang wrote: > On 2015/01/27 20:15:12, spang wrote: > > Do you need this task hop? Aren't we already on UI? > > > > If so you can inline CollectStatusLog() here. > > My bad, I misread the code, TouchLogSource::Fetch is the UI part. This is right. Done. https://codereview.chromium.org/864253002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/system_logs/touch_log_source_ozone.cc:91: ->PostTask(FROM_HERE, base::Bind(&StartLogRetrieval, response, callback)); On 2015/01/27 20:15:13, spang wrote: > Use base::Passed() to pass ownership of response through the callback. This > makes the code a lot clearer and also prevents memory leaks if PostTask fails > and the the callback never executes. Done. https://codereview.chromium.org/864253002/diff/80001/ui/ozone/public/input_co... File ui/ozone/public/input_controller.cc (right): https://codereview.chromium.org/864253002/diff/80001/ui/ozone/public/input_co... ui/ozone/public/input_controller.cc:130: NOTIMPLEMENTED(); On 2015/01/27 20:15:13, spang wrote: > I think you should call the reply to avoid waiting forever. Done.
thanks! one last thing https://codereview.chromium.org/864253002/diff/100001/ui/events/ozone/evdev/i... File ui/events/ozone/evdev/input_controller_evdev.cc (right): https://codereview.chromium.org/864253002/diff/100001/ui/events/ozone/evdev/i... ui/events/ozone/evdev/input_controller_evdev.cc:263: base::WorkerPool::PostTaskAndReply( You cannot dereference EventFactoryEvdev* from the worker pool; it is a UI thread object. So change base::WorkerPool::PostTaskAndReply() to base::ThreadTaskRunnerHandle::Get()->PostTaskAndReply() so that the task runs on UI.
https://codereview.chromium.org/864253002/diff/100001/ui/events/ozone/evdev/i... File ui/events/ozone/evdev/input_controller_evdev.cc (right): https://codereview.chromium.org/864253002/diff/100001/ui/events/ozone/evdev/i... ui/events/ozone/evdev/input_controller_evdev.cc:263: base::WorkerPool::PostTaskAndReply( On 2015/01/27 23:13:08, spang wrote: > You cannot dereference EventFactoryEvdev* from the worker pool; it is a UI > thread object. > > So change base::WorkerPool::PostTaskAndReply() to > base::ThreadTaskRunnerHandle::Get()->PostTaskAndReply() so that the task runs on > UI. Done.
lgtm
The CQ bit was checked by sheckylin@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/864253002/140001
The CQ bit was unchecked by spang@chromium.org
On 2015/01/27 23:45:29, I haz the power (commit-bot) wrote: > CQ is trying da patch. Follow status at > https://chromium-cq-status.appspot.com/patch-status/864253002/140001 looks like you may need to adjust some USE_EVDEV_GESTURES #ifdefs
On 2015/01/28 00:33:00, spang wrote: > On 2015/01/27 23:45:29, I haz the power (commit-bot) wrote: > > CQ is trying da patch. Follow status at > > https://chromium-cq-status.appspot.com/patch-status/864253002/140001 > > looks like you may need to adjust some USE_EVDEV_GESTURES #ifdefs Yeah, fixing it. That always happens.
https://codereview.chromium.org/864253002/diff/160001/ui/events/ozone/evdev/i... File ui/events/ozone/evdev/input_controller_evdev.cc (right): https://codereview.chromium.org/864253002/diff/160001/ui/events/ozone/evdev/i... ui/events/ozone/evdev/input_controller_evdev.cc:266: std::string* status_ptr = status.get(); Probably better to do something like: #if defined(USE_EVDEV_GESTURES) // PostTaskAndReply #else reply.Run(make_scoped_ptr(new std::string))); #endif and just #ifdef out the whole function.
https://codereview.chromium.org/864253002/diff/160001/ui/events/ozone/evdev/i... File ui/events/ozone/evdev/input_controller_evdev.cc (right): https://codereview.chromium.org/864253002/diff/160001/ui/events/ozone/evdev/i... ui/events/ozone/evdev/input_controller_evdev.cc:266: std::string* status_ptr = status.get(); On 2015/01/28 00:50:47, spang wrote: > Probably better to do something like: > > #if defined(USE_EVDEV_GESTURES) > // PostTaskAndReply > #else > reply.Run(make_scoped_ptr(new std::string))); > #endif > > > and just #ifdef out the whole function. Done.
The CQ bit was checked by sheckylin@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/864253002/200001
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/8b1fb4fdf2b13b71e969cd541e500549163f742b Cr-Commit-Position: refs/heads/master@{#313446}
Message was sent while issue was closed.
A revert of this CL (patchset #11 id:200001) has been created in https://codereview.chromium.org/881353002/ by jamescook@chromium.org. The reason for reverting is: Broke the arm-generic-freon builder on Chrome OS: https://uberchromegw.corp.google.com/i/chromeos/builders/arm-generic_freon%20... See crbug.com/452911 - looks like a printf format specifier is wrong on 32 bit .
The CQ bit was checked by sheckylin@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/864253002/220001
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/e09a9b07f189a5ae9cd5f01cba6be9b97e76cad1 Cr-Commit-Position: refs/heads/master@{#313555} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
