Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1949393007:
X11: Better timestamp handling for _NET_ACTIVE_WINDOW (Closed)
|
Created:
4 years, 7 months ago by Tom (Use chromium acct) Modified:
4 years, 7 months ago CC:
chromium-reviews, tfarina, asvitkine+watch_chromium.org, tdresser+watch_chromium.org, piman, pkotwicz, danakj, dcheng Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionX11: Better timestamp handling for _NET_ACTIVE_WINDOW
Main changes introduced:
1. Get the X server timestamp in the newly created browser process and
send it over IPC to the old process to update wm_user_time_ms
2. Add logic in set_wm_user_time_ms to avoid setting the time to 0 (or
backwards)
3. If wm_user_time_ms == 0 and we try to activate a window, grab the
server time first
BUG=593516, 608521, 379615
Committed: https://crrev.com/ca3656dc321be956db788c5fc8ee16cd20ed6d86
Cr-Commit-Position: refs/heads/master@{#394018}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Added XFlush and moved code to chrome_browser_main_extra_parts_x11.cc #
Total comments: 4
Patch Set 3 : Change kAtomStr, fix formatting #
Total comments: 10
Patch Set 4 : Refactor #
Total comments: 1
Patch Set 5 : Fix comment #Patch Set 6 : Merge :P #Patch Set 7 : Don't clobber the root window event mask #
Total comments: 1
Patch Set 8 : Change a property of a new window instead of the root window #
Total comments: 7
Patch Set 9 : C++ style casts and remove dummy_data_ #
Total comments: 4
Patch Set 10 : Move to SCOPED_UMA_HISTOGRAM_TIMER #Patch Set 11 : ms->microseconds #Messages
Total messages: 77 (29 generated)
thomasanderson@google.com changed reviewers: + erg@chromium.org, sky@chromium.org
https://codereview.chromium.org/1949393007/diff/1/chrome/browser/process_sing... File chrome/browser/process_singleton_posix.cc (right): https://codereview.chromium.org/1949393007/diff/1/chrome/browser/process_sing... chrome/browser/process_singleton_posix.cc:831: This doesn't seem like the best place to put this. However, it's the only place where we know another process exists, and before we actually send the arguments. Maybe this could be moved to ChromeBrowserMainParts::PreCreateThreadsImpl? We would need to call UpdateServerTime anyway when we make the first call to Activate() since wm_user_time_ms is initialized to 0. However, then the new --wm-user-time-ms flag will redundantly set the server time again (although that shouldn't matter now that we have the new logic in set_wm_user_time_ms) https://codereview.chromium.org/1949393007/diff/1/ui/events/platform/x11/x11_... File ui/events/platform/x11/x11_event_source.cc (right): https://codereview.chromium.org/1949393007/diff/1/ui/events/platform/x11/x11_... ui/events/platform/x11/x11_event_source.cc:158: XDeleteProperty(display_, window, dummyAtom); For some reason, XDeleteProperty doesn't actually delete the property in the root window (but I can delete it using xprop -root -remove "xyz"). Anyone know why this might be happening?
https://codereview.chromium.org/1949393007/diff/1/chrome/browser/process_sing... File chrome/browser/process_singleton_posix.cc (right): https://codereview.chromium.org/1949393007/diff/1/chrome/browser/process_sing... chrome/browser/process_singleton_posix.cc:831: On 2016/05/06 20:42:00, Tom Anderson wrote: > This doesn't seem like the best place to put this. However, it's the only place > where we know another process exists, and before we actually send the arguments. > > Maybe this could be moved to ChromeBrowserMainParts::PreCreateThreadsImpl? We > would need to call UpdateServerTime anyway when we make the first call to > Activate() since wm_user_time_ms is initialized to 0. However, then the new > --wm-user-time-ms flag will redundantly set the server time again (although that > shouldn't matter now that we have the new logic in set_wm_user_time_ms) I agree that this is really not a good place for this, and putting this in something like ChromeBrowserMainExtraPartsX11 would be a better place for this to live.
piman@chromium.org changed reviewers: + piman@chromium.org
https://codereview.chromium.org/1949393007/diff/1/ui/events/platform/x11/x11_... File ui/events/platform/x11/x11_event_source.cc (right): https://codereview.chromium.org/1949393007/diff/1/ui/events/platform/x11/x11_... ui/events/platform/x11/x11_event_source.cc:158: XDeleteProperty(display_, window, dummyAtom); On 2016/05/06 20:42:00, Tom Anderson wrote: > For some reason, XDeleteProperty doesn't actually delete the property in the > root window (but I can delete it using xprop -root -remove "xyz"). Anyone know > why this might be happening? Maybe need an XFlush ?
https://codereview.chromium.org/1949393007/diff/1/chrome/browser/process_sing... File chrome/browser/process_singleton_posix.cc (right): https://codereview.chromium.org/1949393007/diff/1/chrome/browser/process_sing... chrome/browser/process_singleton_posix.cc:831: On 2016/05/06 21:13:13, Elliot Glaysher wrote: > On 2016/05/06 20:42:00, Tom Anderson wrote: > > This doesn't seem like the best place to put this. However, it's the only > place > > where we know another process exists, and before we actually send the > arguments. > > > > Maybe this could be moved to ChromeBrowserMainParts::PreCreateThreadsImpl? We > > would need to call UpdateServerTime anyway when we make the first call to > > Activate() since wm_user_time_ms is initialized to 0. However, then the new > > --wm-user-time-ms flag will redundantly set the server time again (although > that > > shouldn't matter now that we have the new logic in set_wm_user_time_ms) > > I agree that this is really not a good place for this, and putting this in > something like ChromeBrowserMainExtraPartsX11 would be a better place for this > to live. Done. https://codereview.chromium.org/1949393007/diff/1/ui/events/platform/x11/x11_... File ui/events/platform/x11/x11_event_source.cc (right): https://codereview.chromium.org/1949393007/diff/1/ui/events/platform/x11/x11_... ui/events/platform/x11/x11_event_source.cc:158: XDeleteProperty(display_, window, dummyAtom); On 2016/05/06 21:23:00, piman wrote: > On 2016/05/06 20:42:00, Tom Anderson wrote: > > For some reason, XDeleteProperty doesn't actually delete the property in the > > root window (but I can delete it using xprop -root -remove "xyz"). Anyone > know > > why this might be happening? > > Maybe need an XFlush ? That worked! Done.
https://codereview.chromium.org/1949393007/diff/20001/chrome/browser/ui/start... File chrome/browser/ui/startup/startup_browser_creator.cc (right): https://codereview.chromium.org/1949393007/diff/20001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator.cc:729: static_cast<Time>(time)); nit: if statements with multi-line bodies get wrapped in {}. https://codereview.chromium.org/1949393007/diff/20001/ui/events/platform/x11/... File ui/events/platform/x11/x11_event_source.cc (right): https://codereview.chromium.org/1949393007/diff/20001/ui/events/platform/x11/... ui/events/platform/x11/x11_event_source.cc:142: const char kAtomStr[] = __FILE__ " CHROMIUM_DUMMY_ATOM"; Are spaces allowed in atoms? Probably want to change that ' ' to a '_'. What about the '/' in __FILE__?
https://codereview.chromium.org/1949393007/diff/20001/chrome/browser/ui/start... File chrome/browser/ui/startup/startup_browser_creator.cc (right): https://codereview.chromium.org/1949393007/diff/20001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator.cc:729: static_cast<Time>(time)); On 2016/05/06 22:21:09, Elliot Glaysher wrote: > nit: if statements with multi-line bodies get wrapped in {}. Done. https://codereview.chromium.org/1949393007/diff/20001/ui/events/platform/x11/... File ui/events/platform/x11/x11_event_source.cc (right): https://codereview.chromium.org/1949393007/diff/20001/ui/events/platform/x11/... ui/events/platform/x11/x11_event_source.cc:142: const char kAtomStr[] = __FILE__ " CHROMIUM_DUMMY_ATOM"; On 2016/05/06 22:21:10, Elliot Glaysher wrote: > Are spaces allowed in atoms? Probably want to change that ' ' to a '_'. What > about the '/' in __FILE__? The doc says atom names are ISO Latin-1 strings. Changed it to something well-defined just in case
lgtm
https://codereview.chromium.org/1949393007/diff/40001/chrome/browser/ui/start... File chrome/browser/ui/startup/startup_browser_creator.cc (right): https://codereview.chromium.org/1949393007/diff/40001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator.cc:725: std::string switchValue = switch_value https://codereview.chromium.org/1949393007/diff/40001/ui/events/platform/x11/... File ui/events/platform/x11/x11_event_source.cc (right): https://codereview.chromium.org/1949393007/diff/40001/ui/events/platform/x11/... ui/events/platform/x11/x11_event_source.cc:131: static Bool is_property_notify_for_timestamp(Display* display, Move into anonymous namespace above, and no static, and if you can make arts const where appropriate. https://codereview.chromium.org/1949393007/diff/40001/ui/events/platform/x11/... ui/events/platform/x11/x11_event_source.cc:139: Time X11EventSource::UpdateServerTime() { UpdateLastSeenServerTime? https://codereview.chromium.org/1949393007/diff/40001/ui/events/platform/x11/... ui/events/platform/x11/x11_event_source.cc:146: const unsigned char dummyData = 0; dummy_data and dummy_atom. Also, please add comments here as it isn't at all obvious why this code is here. https://codereview.chromium.org/1949393007/diff/40001/ui/events/platform/x11/... File ui/events/platform/x11/x11_event_source.h (right): https://codereview.chromium.org/1949393007/diff/40001/ui/events/platform/x11/... ui/events/platform/x11/x11_event_source.h:65: Time UpdateServerTime(); Add description.
https://codereview.chromium.org/1949393007/diff/40001/chrome/browser/ui/start... File chrome/browser/ui/startup/startup_browser_creator.cc (right): https://codereview.chromium.org/1949393007/diff/40001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator.cc:725: std::string switchValue = On 2016/05/07 16:58:34, sky wrote: > switch_value Done. https://codereview.chromium.org/1949393007/diff/40001/ui/events/platform/x11/... File ui/events/platform/x11/x11_event_source.cc (right): https://codereview.chromium.org/1949393007/diff/40001/ui/events/platform/x11/... ui/events/platform/x11/x11_event_source.cc:131: static Bool is_property_notify_for_timestamp(Display* display, On 2016/05/07 16:58:34, sky wrote: > Move into anonymous namespace above, and no static, and if you can make arts > const where appropriate. Done. https://codereview.chromium.org/1949393007/diff/40001/ui/events/platform/x11/... ui/events/platform/x11/x11_event_source.cc:139: Time X11EventSource::UpdateServerTime() { On 2016/05/07 16:58:34, sky wrote: > UpdateLastSeenServerTime? Done. https://codereview.chromium.org/1949393007/diff/40001/ui/events/platform/x11/... ui/events/platform/x11/x11_event_source.cc:146: const unsigned char dummyData = 0; On 2016/05/07 16:58:34, sky wrote: > dummy_data and dummy_atom. Also, please add comments here as it isn't at all > obvious why this code is here. Done. https://codereview.chromium.org/1949393007/diff/40001/ui/events/platform/x11/... File ui/events/platform/x11/x11_event_source.h (right): https://codereview.chromium.org/1949393007/diff/40001/ui/events/platform/x11/... ui/events/platform/x11/x11_event_source.h:65: Time UpdateServerTime(); On 2016/05/07 16:58:34, sky wrote: > Add description. Done.
LGTM https://codereview.chromium.org/1949393007/diff/60001/ui/events/platform/x11/... File ui/events/platform/x11/x11_event_source.h (right): https://codereview.chromium.org/1949393007/diff/60001/ui/events/platform/x11/... ui/events/platform/x11/x11_event_source.h:66: // last_seen_server_time with this value. nit: generally when referring to fields and parameters wrap it in |s, eg |last_seen_server_time_|
The CQ bit was checked by thomasanderson@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from erg@chromium.org Link to the patchset: https://codereview.chromium.org/1949393007/#ps60001 (title: "Refactor")
The CQ bit was unchecked by thomasanderson@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1949393007/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1949393007/60001
The CQ bit was checked by thomasanderson@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from erg@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/1949393007/#ps80001 (title: "Fix comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1949393007/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1949393007/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-device on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
Patchset #6 (id:100001) has been deleted
The CQ bit was checked by thomasanderson@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1949393007/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1949393007/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by thomasanderson@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1949393007/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1949393007/140001
Elliot/Dana PTAL at the deltas from the latest patchset before commit https://codereview.chromium.org/1949393007/diff2/120001:140001/ui/events/plat...
danakj@chromium.org changed reviewers: + danakj@chromium.org
https://codereview.chromium.org/1949393007/diff/140001/ui/events/platform/x11... File ui/events/platform/x11/x11_event_source.cc (right): https://codereview.chromium.org/1949393007/diff/140001/ui/events/platform/x11... ui/events/platform/x11/x11_event_source.cc:158: XGetWindowAttributes(display_, window, &attributes); Round trips :( Can you just use some arbitrary never-mapped window to do this timestamp generation instead of the root?
On 2016/05/12 21:32:50, danakj wrote: > https://codereview.chromium.org/1949393007/diff/140001/ui/events/platform/x11... > File ui/events/platform/x11/x11_event_source.cc (right): > > https://codereview.chromium.org/1949393007/diff/140001/ui/events/platform/x11... > ui/events/platform/x11/x11_event_source.cc:158: XGetWindowAttributes(display_, > window, &attributes); > Round trips :( > > Can you just use some arbitrary never-mapped window to do this timestamp > generation instead of the root? That sounds much safer. Is it expensive to create a temporary window that we never map?
On Thu, May 12, 2016 at 2:35 PM, <thomasanderson@google.com> wrote: > On 2016/05/12 21:32:50, danakj wrote: > > > > https://codereview.chromium.org/1949393007/diff/140001/ui/events/platform/x11... > > File ui/events/platform/x11/x11_event_source.cc (right): > > > > > > https://codereview.chromium.org/1949393007/diff/140001/ui/events/platform/x11... > > ui/events/platform/x11/x11_event_source.cc:158: > XGetWindowAttributes(display_, > > window, &attributes); > > Round trips :( > > > > Can you just use some arbitrary never-mapped window to do this timestamp > > generation instead of the root? > > That sounds much safer. Is it expensive to create a temporary window that > we > never map? > Nah I think it's fine. Just make one at startup or the first time you want it and use it forever. -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by thomasanderson@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1949393007/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1949393007/160001
On 2016/05/12 21:39:09, danakj wrote: > On Thu, May 12, 2016 at 2:35 PM, <mailto:thomasanderson@google.com> wrote: > > > On 2016/05/12 21:32:50, danakj wrote: > > > > > > > > https://codereview.chromium.org/1949393007/diff/140001/ui/events/platform/x11... > > > File ui/events/platform/x11/x11_event_source.cc (right): > > > > > > > > > > > https://codereview.chromium.org/1949393007/diff/140001/ui/events/platform/x11... > > > ui/events/platform/x11/x11_event_source.cc:158: > > XGetWindowAttributes(display_, > > > window, &attributes); > > > Round trips :( > > > > > > Can you just use some arbitrary never-mapped window to do this timestamp > > > generation instead of the root? > > > > That sounds much safer. Is it expensive to create a temporary window that > > we > > never map? > > > > Nah I think it's fine. Just make one at startup or the first time you want > it and use it forever. Doing the property change on a new window now. This actually reduced the runtime of UpdateLastSeenServerTime from 300us to 250us. And with the window caching, subsequent calls are 125us.
That looks nicer, thanks, 2 comments: https://codereview.chromium.org/1949393007/diff/160001/ui/events/platform/x11... File ui/events/platform/x11/x11_event_source.cc (right): https://codereview.chromium.org/1949393007/diff/160001/ui/events/platform/x11... ui/events/platform/x11/x11_event_source.cc:159: dummy_atom_ = XInternAtom(display_, "TIMESTAMP", False); Can you namespace this like CHROMIUM_TIMESTAMP https://codereview.chromium.org/1949393007/diff/160001/ui/events/platform/x11... ui/events/platform/x11/x11_event_source.cc:166: XChangeProperty(display_, dummy_window_, dummy_atom_, XA_INTEGER, 8, I believe you can do this and avoid the need for dummy_data_: XChangeProperty(display_, dummy_window_, dummy_atom_, XA_STRING, 8, PropModeAppend, nullptr, 0);
https://codereview.chromium.org/1949393007/diff/160001/ui/events/platform/x11... File ui/events/platform/x11/x11_event_source.cc (right): https://codereview.chromium.org/1949393007/diff/160001/ui/events/platform/x11... ui/events/platform/x11/x11_event_source.cc:87: event->xproperty.window == *(Window*)arg; C++ style casts please https://codereview.chromium.org/1949393007/diff/160001/ui/events/platform/x11... ui/events/platform/x11/x11_event_source.cc:172: (XPointer)&dummy_window_); Oh, c++-style casts please.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Going to commit after a dry run https://codereview.chromium.org/1949393007/diff/160001/ui/events/platform/x11... File ui/events/platform/x11/x11_event_source.cc (right): https://codereview.chromium.org/1949393007/diff/160001/ui/events/platform/x11... ui/events/platform/x11/x11_event_source.cc:87: event->xproperty.window == *(Window*)arg; On 2016/05/12 23:35:48, danakj wrote: > C++ style casts please Done. https://codereview.chromium.org/1949393007/diff/160001/ui/events/platform/x11... ui/events/platform/x11/x11_event_source.cc:159: dummy_atom_ = XInternAtom(display_, "TIMESTAMP", False); On 2016/05/12 23:33:26, danakj wrote: > Can you namespace this like CHROMIUM_TIMESTAMP Done. https://codereview.chromium.org/1949393007/diff/160001/ui/events/platform/x11... ui/events/platform/x11/x11_event_source.cc:166: XChangeProperty(display_, dummy_window_, dummy_atom_, XA_INTEGER, 8, On 2016/05/12 23:33:26, danakj wrote: > I believe you can do this and avoid the need for dummy_data_: > > XChangeProperty(display_, dummy_window_, dummy_atom_, XA_STRING, 8, > PropModeAppend, nullptr, 0); Done. Also, cool!
The CQ bit was checked by thomasanderson@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1949393007/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1949393007/180001
Thanks that LGTM
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by thomasanderson@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from erg@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/1949393007/#ps180001 (title: "C++ style casts and remove dummy_data_")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1949393007/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1949393007/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
thomasanderson@google.com changed reviewers: + rkaplow@chromium.org
Pinging rkaplow@ for OWNERS permission for histograms.xml
https://codereview.chromium.org/1949393007/diff/180001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1949393007/diff/180001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:12336: + units="microseconds"> this is in milliseconds actually https://codereview.chromium.org/1949393007/diff/180001/ui/events/platform/x11... File ui/events/platform/x11/x11_event_source.cc (right): https://codereview.chromium.org/1949393007/diff/180001/ui/events/platform/x11... ui/events/platform/x11/x11_event_source.cc:174: UMA_HISTOGRAM_TIMES("Event.Latency.X11EventSource.UpdateServerTime", you can used SCOPED_UMA_HISTOGRAM_TIMER if you'd like
https://codereview.chromium.org/1949393007/diff/180001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1949393007/diff/180001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:12336: + units="microseconds"> On 2016/05/13 21:30:44, rkaplow wrote: > this is in milliseconds actually Done. But I'm not sure I understand. When I manually printed timediff.InMicroseconds(), I was getting between 100us and 300us. https://codereview.chromium.org/1949393007/diff/180001/ui/events/platform/x11... File ui/events/platform/x11/x11_event_source.cc (right): https://codereview.chromium.org/1949393007/diff/180001/ui/events/platform/x11... ui/events/platform/x11/x11_event_source.cc:174: UMA_HISTOGRAM_TIMES("Event.Latency.X11EventSource.UpdateServerTime", On 2016/05/13 21:30:44, rkaplow wrote: > you can used SCOPED_UMA_HISTOGRAM_TIMER if you'd like Done.
The CQ bit was checked by thomasanderson@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1949393007/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1949393007/200001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/05/14 04:18:39, Tom Anderson wrote: > https://codereview.chromium.org/1949393007/diff/180001/tools/metrics/histogra... > File tools/metrics/histograms/histograms.xml (right): > > https://codereview.chromium.org/1949393007/diff/180001/tools/metrics/histogra... > tools/metrics/histograms/histograms.xml:12336: + units="microseconds"> > On 2016/05/13 21:30:44, rkaplow wrote: > > this is in milliseconds actually > > Done. But I'm not sure I understand. When I manually printed > timediff.InMicroseconds(), I was getting between 100us and 300us. > > https://codereview.chromium.org/1949393007/diff/180001/ui/events/platform/x11... > File ui/events/platform/x11/x11_event_source.cc (right): > > https://codereview.chromium.org/1949393007/diff/180001/ui/events/platform/x11... > ui/events/platform/x11/x11_event_source.cc:174: > UMA_HISTOGRAM_TIMES("Event.Latency.X11EventSource.UpdateServerTime", > On 2016/05/13 21:30:44, rkaplow wrote: > > you can used SCOPED_UMA_HISTOGRAM_TIMER if you'd like > > Done. well, the default macros will be in milliseconds. If you want to measure in microseconds you have to set it up yourself, something like: https://code.google.com/p/chromium/codesearch#chromium/src/components/variati...
On 2016/05/16 14:49:34, rkaplow wrote: Can I instead use a timediff, specifying microseconds in histograms.xml like in this example: https://code.google.com/p/chromium/codesearch#chromium/src/tools/metrics/hist... https://code.google.com/p/chromium/codesearch#chromium/src/components/drive/c...
On 2016/05/16 16:25:50, Tom Anderson wrote: > On 2016/05/16 14:49:34, rkaplow wrote: > > Can I instead use a timediff, specifying microseconds in histograms.xml like in > this example: > https://code.google.com/p/chromium/codesearch#chromium/src/tools/metrics/hist... > https://code.google.com/p/chromium/codesearch#chromium/src/components/drive/c... Right - that code is actually incorrect. Even though the unit says us, the macro used converts the time to milliseconds, see https://code.google.com/p/chromium/codesearch#chromium/src/base/metrics/histo... I'll file a bug for them Best is to do what I suggested as per: https://code.google.com/p/chromium/codesearch#chromium/src/components/variati...
The CQ bit was checked by thomasanderson@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1949393007/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1949393007/220001
On 2016/05/16 19:39:54, rkaplow wrote: > On 2016/05/16 16:25:50, Tom Anderson wrote: > > On 2016/05/16 14:49:34, rkaplow wrote: > > > > Can I instead use a timediff, specifying microseconds in histograms.xml like > in > > this example: > > > https://code.google.com/p/chromium/codesearch#chromium/src/tools/metrics/hist... > > > https://code.google.com/p/chromium/codesearch#chromium/src/components/drive/c... > > Right - that code is actually incorrect. Even though the unit says us, the macro > used converts the time to milliseconds, see > > https://code.google.com/p/chromium/codesearch#chromium/src/base/metrics/histo... > > I'll file a bug for them > > Best is to do what I suggested as per: > https://code.google.com/p/chromium/codesearch#chromium/src/components/variati... Changed to microseconds
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by thomasanderson@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org, erg@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/1949393007/#ps220001 (title: "ms->microseconds")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1949393007/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1949393007/220001
Message was sent while issue was closed.
Committed patchset #11 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== X11: Better timestamp handling for _NET_ACTIVE_WINDOW Main changes introduced: 1. Get the X server timestamp in the newly created browser process and send it over IPC to the old process to update wm_user_time_ms 2. Add logic in set_wm_user_time_ms to avoid setting the time to 0 (or backwards) 3. If wm_user_time_ms == 0 and we try to activate a window, grab the server time first BUG=593516,608521,379615 ========== to ========== X11: Better timestamp handling for _NET_ACTIVE_WINDOW Main changes introduced: 1. Get the X server timestamp in the newly created browser process and send it over IPC to the old process to update wm_user_time_ms 2. Add logic in set_wm_user_time_ms to avoid setting the time to 0 (or backwards) 3. If wm_user_time_ms == 0 and we try to activate a window, grab the server time first BUG=593516,608521,379615 Committed: https://crrev.com/ca3656dc321be956db788c5fc8ee16cd20ed6d86 Cr-Commit-Position: refs/heads/master@{#394018} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/ca3656dc321be956db788c5fc8ee16cd20ed6d86 Cr-Commit-Position: refs/heads/master@{#394018} |
