Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1892483005:
SelectionOwner: add support for TIMESTAMP target (Closed)
|
Created:
4 years, 8 months ago by dcheng Modified:
4 years, 8 months ago CC:
chromium-reviews, danakj, derat+watch_chromium.org, sadrul, yusukes+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSelectionOwner: add support for TIMESTAMP target
Even though XSetSelectionOwner() has a Time argument, there's no way to
retrieve the timestamp associated with the selection from the X server
itself. Instead, the X11 selection protocol requires selections to
provide a TIMESTAMP target.
Unfortunately, there's also no good way to get the current timestamp
from the X server: one way is to make a no-op property change and
observe the resulting PropertyNotify event. This is the what GTK does,
but it both blocks and performs out-of-order event dispatching.
In Chromium, we control our event source, so we implement an alternate
strategy instead: we record the server time seen in X events with a
time field and use the last seen value when taking ownership of the X
selection. Of course, since this is X, the server time observer has to
be careful to ignore instances where time == CurrentTime (explicitly
set by some programs to indicate an X event is synthesized) and where
time goes backwards (since events can be reordered).
Note that despite all this logic to sanitize X server time, actually
using the last seen time when calling XSetSelectionOwner() doesn't
actually seem to be good enough. Random omnibox tests still fail in
that case, so we still pass CurrentTime to XSetSelectionOwner() to
increase the chances of success.
¯\_(ツ)_/¯
BUG=395077
Committed: https://crrev.com/5c42da4d3ec29ffaf0bb9d3fbd33a55f75249781
Cr-Commit-Position: refs/heads/master@{#389208}
Patch Set 1 #Patch Set 2 : I hate X #
Total comments: 2
Patch Set 3 : . #Patch Set 4 : . #
Total comments: 4
Patch Set 5 : . #
Total comments: 2
Patch Set 6 : Fix gyp? #Patch Set 7 : ugh #
Total comments: 2
Patch Set 8 : nogncheck #
Total comments: 12
Patch Set 9 : address feedback #
Total comments: 2
Patch Set 10 : USE_GLIB #
Messages
Total messages: 49 (19 generated)
Description was changed from ========== Selectionowner: add support for TIMESTAMP target BUG=395077 ========== to ========== SelectionOwner: add support for TIMESTAMP target BUG=395077 ==========
dcheng@chromium.org changed reviewers: + sadrul@chromium.org
Behold the beauty and elegance of the X selection protocol.
cc lambroslambrou@ as well, since I think there's a long-standing TODO in remoting's X server clipboard support: maybe there's some opportunity for future cleanup / code sharing.
erg@chromium.org changed reviewers: + erg@chromium.org
https://codereview.chromium.org/1892483005/diff/20001/ui/base/x/selection_own... File ui/base/x/selection_owner.cc (right): https://codereview.chromium.org/1892483005/diff/20001/ui/base/x/selection_own... ui/base/x/selection_owner.cc:82: XIfEvent(display, &event, +predicate, reinterpret_cast<XPointer>(&args)); "+" predicate? I assume the unary plus is some sort of cast?
https://codereview.chromium.org/1892483005/diff/20001/ui/base/x/selection_own... File ui/base/x/selection_owner.cc (right): https://codereview.chromium.org/1892483005/diff/20001/ui/base/x/selection_own... ui/base/x/selection_owner.cc:82: XIfEvent(display, &event, +predicate, reinterpret_cast<XPointer>(&args)); On 2016/04/15 at 21:45:31, Elliot Glaysher wrote: > "+" predicate? I assume the unary plus is some sort of cast? I was forcing a decay to function pointer, but it turns out that it's not necessary.
Ack, it looks like there's some failures with DND. Let me look at those
PTAL, I think it should be fixed now. The problem is that OSExchangeDataProviderAuraX11's X11 window wasn't listening to property notify events. FWIW, there are some alternatives: 1) Don't bother supporting TIMESTAMP for drag-and-drop, since it doesn't appear to be required. But this requires plumbing in a bool somewhere to tell SelectionOwner about this policy for drag and drop. 2) Make Time an optional field of the format map. But this seemed kind of ugly to plumb through: we don't actually have base::Optional yet, and storing the int as ref-counted data doesn't feel right either. 3) Modify X11EventSource to store the last seen server time and just always use that. In the end, I stuck with the XIfEvent solution since it's relatively 'self-contained'.
https://codereview.chromium.org/1892483005/diff/60001/ui/base/x/selection_own... File ui/base/x/selection_owner.cc (right): https://codereview.chromium.org/1892483005/diff/60001/ui/base/x/selection_own... ui/base/x/selection_owner.cc:83: return event.xproperty.time; How does things break when we use CurrentTime? We have had enough fun with events being processed out of order, or disappearing. I wouldn't want to complicate things farther. I am tempted to just live with whatever brokenness we get with CurrentTime (or even base::Time::Now()/UnixEpoch() etc.) instead.
https://codereview.chromium.org/1892483005/diff/60001/ui/base/x/selection_own... File ui/base/x/selection_owner.cc (right): https://codereview.chromium.org/1892483005/diff/60001/ui/base/x/selection_own... ui/base/x/selection_owner.cc:83: return event.xproperty.time; On 2016/04/19 at 16:11:52, sadrul wrote: > How does things break when we use CurrentTime? > > We have had enough fun with events being processed out of order, or disappearing. I wouldn't want to complicate things farther. I am tempted to just live with whatever brokenness we get with CurrentTime (or even base::Time::Now()/UnixEpoch() etc.) instead. Just using CurrentTime is unlikely to actually resolve the issue seen by the bug reporters: VMWare uses the TIMESTAMP target to determine which selection was more recently set and needs to be forwarded to a VM. base::Time::Now() / UnixEpoch() have similar issues. This seems like a pretty reasonable complaint to me, especially since it used to work just fine before we switched to Aura =) Processing events out of order is kind of lame, but in this particular case, it's pretty narrowly scoped: the only event we should pull out of the event queue is the one we triggered ourselves. The alternative, as I mentioned, is to save the timestamp off events in X11EventSource or the like.
https://codereview.chromium.org/1892483005/diff/60001/ui/base/x/selection_own... File ui/base/x/selection_owner.cc (right): https://codereview.chromium.org/1892483005/diff/60001/ui/base/x/selection_own... ui/base/x/selection_owner.cc:83: return event.xproperty.time; On 2016/04/19 17:24:15, dcheng wrote: > Processing events out of order is kind of lame, but in this particular case, > it's pretty narrowly scoped: the only event we should pull out of the event > queue is the one we triggered ourselves. The alternative, as I mentioned, is to > save the timestamp off events in X11EventSource or the like. Sadly, I wouldn't rely on that. We currently have a bug open where we're getting MapNotify messages that don't correspond to us calling XMapWindow(). Saving the timestamps off X11EventSource sounds significantly safer.
https://codereview.chromium.org/1892483005/diff/60001/ui/base/x/selection_own... File ui/base/x/selection_owner.cc (right): https://codereview.chromium.org/1892483005/diff/60001/ui/base/x/selection_own... ui/base/x/selection_owner.cc:83: return event.xproperty.time; On 2016/04/19 at 17:30:36, Elliot Glaysher wrote: > On 2016/04/19 17:24:15, dcheng wrote: > > Processing events out of order is kind of lame, but in this particular case, > > it's pretty narrowly scoped: the only event we should pull out of the event > > queue is the one we triggered ourselves. The alternative, as I mentioned, is to > > save the timestamp off events in X11EventSource or the like. > > Sadly, I wouldn't rely on that. We currently have a bug open where we're getting MapNotify messages that don't correspond to us calling XMapWindow(). Saving the timestamps off X11EventSource sounds significantly safer. Is the concern here that some other app will XChangeProprety on our dummy property and cause us to mistakenly pull the wrong event out of the queue? Or that we'll just hang like the MapNotify bug (assuming I'm looking at the right one)?
On 2016/04/19 17:44:11, dcheng wrote: > https://codereview.chromium.org/1892483005/diff/60001/ui/base/x/selection_own... > File ui/base/x/selection_owner.cc (right): > > https://codereview.chromium.org/1892483005/diff/60001/ui/base/x/selection_own... > ui/base/x/selection_owner.cc:83: return event.xproperty.time; > On 2016/04/19 at 17:30:36, Elliot Glaysher wrote: > > On 2016/04/19 17:24:15, dcheng wrote: > > > Processing events out of order is kind of lame, but in this particular case, > > > it's pretty narrowly scoped: the only event we should pull out of the event > > > queue is the one we triggered ourselves. The alternative, as I mentioned, is > to > > > save the timestamp off events in X11EventSource or the like. > > > > Sadly, I wouldn't rely on that. We currently have a bug open where we're > getting MapNotify messages that don't correspond to us calling XMapWindow(). > Saving the timestamps off X11EventSource sounds significantly safer. > > Is the concern here that some other app will XChangeProprety on our dummy > property and cause us to mistakenly pull the wrong event out of the queue? Or > that we'll just hang like the MapNotify bug (assuming I'm looking at the right > one)? Blocking is bad in general, but the greater point I'm trying to make is that I don't believe we can rely on other apps to follow the various x11 specs in ways that we rely on. If there's a way to do things locally without exposing surface to the swirling chaos that is an x connection, I'd prefer that.
https://codereview.chromium.org/1892483005/diff/80001/ui/events/platform/x11/... File ui/events/platform/x11/x11_event_source.cc (right): https://codereview.chromium.org/1892483005/diff/80001/ui/events/platform/x11/... ui/events/platform/x11/x11_event_source.cc:133: void X11EventSource::ExtractTimeFromXEvent(const XEvent& xevent) { An alternative was to stash the timestamp off https://code.google.com/p/chromium/codesearch#chromium/src/ui/events/x/events..., but that felt more fragile. One other possibility I considered is to refactor that function so there's a Time and base::TimeDelta version, but that makes the TimeDelta version a bit awkward for GenericEvent handling. So in the end, I just forked it. https://codereview.chromium.org/1892483005/diff/80001/ui/events/platform/x11/... ui/events/platform/x11/x11_event_source.cc:163: last_seen_server_time_ = I have no idea how this is safe, but other code seems to do this, e.g. https://code.google.com/p/chromium/codesearch#chromium/src/ui/events/x/events...
The CQ bit was checked by dcheng@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1892483005/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1892483005/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_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 dcheng@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1892483005/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1892483005/120001
PTAL. Added a few more hacks, because X. https://codereview.chromium.org/1892483005/diff/120001/components/bookmarks/b... File components/bookmarks/browser/bookmark_utils_unittest.cc (right): https://codereview.chromium.org/1892483005/diff/120001/components/bookmarks/b... components/bookmarks/browser/bookmark_utils_unittest.cc:91: ui::X11EventSourceGlib event_source_; I'm pretty sure the existing bookmark clipboard tests are flaky. See https://crbug.com/396472 for instance. https://codereview.chromium.org/1892483005/diff/120001/ui/base/x/selection_ow... File ui/base/x/selection_owner.cc (right): https://codereview.chromium.org/1892483005/diff/120001/ui/base/x/selection_ow... ui/base/x/selection_owner.cc:127: XSetSelectionOwner(x_display_, selection_name_, x_window_, CurrentTime); For whatever reason, using last_seen_server_time() causes XSetSelectionOwner() to fail randomly. So even though we grab a timestamp from X, we always pass CurrentTime here to make it more likely that it will succeed.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by dcheng@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1892483005/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1892483005/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thank you for humouring me. LGTM. https://codereview.chromium.org/1892483005/diff/140001/components/bookmarks/b... File components/bookmarks/browser/bookmark_utils_unittest.cc (right): https://codereview.chromium.org/1892483005/diff/140001/components/bookmarks/b... components/bookmarks/browser/bookmark_utils_unittest.cc:43: event_source_(gfx::GetXDisplay()) { I believe this is supposed to be: other_thing() #if defined(USE_X11) , event_source...() #endif {} Also, this part is really unfortunate. :-/
https://codereview.chromium.org/1892483005/diff/140001/ui/base/x/selection_ow... File ui/base/x/selection_owner.cc (right): https://codereview.chromium.org/1892483005/diff/140001/ui/base/x/selection_ow... ui/base/x/selection_owner.cc:124: X11EventSource::GetInstance()->last_seen_server_time(); Can this use something else (like CurrentTime) if X11EventSource doesn't exist? That way the other tests that don't care don't need to create an X11EventSource thingy https://codereview.chromium.org/1892483005/diff/140001/ui/base/x/selection_ow... ui/base/x/selection_owner.cc:126: // of this succeeding. um, lol? https://codereview.chromium.org/1892483005/diff/140001/ui/base/x/selection_ow... ui/base/x/selection_owner.cc:234: } else if (target == targets_atom) { Don't need the else. https://codereview.chromium.org/1892483005/diff/140001/ui/events/platform/x11... File ui/events/platform/x11/x11_event_source.cc (right): https://codereview.chromium.org/1892483005/diff/140001/ui/events/platform/x11... ui/events/platform/x11/x11_event_source.cc:135: Time X11EventSource::ExtractTimeFromXEvent(const XEvent& xevent) { This can go in a standalone function in anon namespace.
https://codereview.chromium.org/1892483005/diff/140001/ui/events/platform/x11... File ui/events/platform/x11/x11_event_source.cc (right): https://codereview.chromium.org/1892483005/diff/140001/ui/events/platform/x11... ui/events/platform/x11/x11_event_source.cc:157: return static_cast<XIDeviceEvent*>(xevent.xcookie.data)->time; Also, you should check DeviceDataManagerX11::IsXIDeviceEvent() before casting to XIDeviceEvent
dcheng@chromium.org changed reviewers: + sky@chromium.org
+sky for //components/bookmarks as well I made two other changes: - Made it so the server timestamp doesn't go backwards, if we somehow process the events out of order. - Added rollover detection similar to the bits in https://code.google.com/p/chromium/codesearch#chromium/src/ui/events/x/events..., so we still update the timestamp if we think it's due to a clock rollover. https://codereview.chromium.org/1892483005/diff/140001/components/bookmarks/b... File components/bookmarks/browser/bookmark_utils_unittest.cc (right): https://codereview.chromium.org/1892483005/diff/140001/components/bookmarks/b... components/bookmarks/browser/bookmark_utils_unittest.cc:43: event_source_(gfx::GetXDisplay()) { On 2016/04/20 at 17:37:22, Elliot Glaysher wrote: > I believe this is supposed to be: > > other_thing() > #if defined(USE_X11) > , event_source...() > #endif > {} > > Also, this part is really unfortunate. :-/ clang-format mangles this pretty horrifically. I've updated this to just use a unique_ptr, so I can set it in the body of the constructor instead. https://codereview.chromium.org/1892483005/diff/140001/ui/base/x/selection_ow... File ui/base/x/selection_owner.cc (right): https://codereview.chromium.org/1892483005/diff/140001/ui/base/x/selection_ow... ui/base/x/selection_owner.cc:124: X11EventSource::GetInstance()->last_seen_server_time(); On 2016/04/20 at 17:58:43, sadrul wrote: > Can this use something else (like CurrentTime) if X11EventSource doesn't exist? That way the other tests that don't care don't need to create an X11EventSource thingy =/ I could, but I'm not a fan of adding checks for conditions that are only true in tests. I'd strongly prefer this to break in production binaries if the preconditions I'm assuming (the existence of an event source) are no longer true. https://codereview.chromium.org/1892483005/diff/140001/ui/base/x/selection_ow... ui/base/x/selection_owner.cc:126: // of this succeeding. On 2016/04/20 at 17:58:43, sadrul wrote: > um, lol? I couldn't figure out why, but without doing this, the omnibox copy/cut tests didn't work. This seemed like the most expedient solution =/ https://codereview.chromium.org/1892483005/diff/140001/ui/base/x/selection_ow... ui/base/x/selection_owner.cc:234: } else if (target == targets_atom) { On 2016/04/20 at 17:58:43, sadrul wrote: > Don't need the else. Done. I updated the rest of this function to be consistent as well. https://codereview.chromium.org/1892483005/diff/140001/ui/events/platform/x11... File ui/events/platform/x11/x11_event_source.cc (right): https://codereview.chromium.org/1892483005/diff/140001/ui/events/platform/x11... ui/events/platform/x11/x11_event_source.cc:135: Time X11EventSource::ExtractTimeFromXEvent(const XEvent& xevent) { On 2016/04/20 at 17:58:43, sadrul wrote: > This can go in a standalone function in anon namespace. Done. https://codereview.chromium.org/1892483005/diff/140001/ui/events/platform/x11... ui/events/platform/x11/x11_event_source.cc:157: return static_cast<XIDeviceEvent*>(xevent.xcookie.data)->time; On 2016/04/20 at 18:01:08, sadrul wrote: > Also, you should check DeviceDataManagerX11::IsXIDeviceEvent() before casting to XIDeviceEvent Done. But as mentioned earlier, other code often doesn't do this (e.g. https://code.google.com/p/chromium/codesearch#chromium/src/ui/aura/window_tre...). How is that code safe?
LGTM
lgtm https://codereview.chromium.org/1892483005/diff/160001/components/bookmarks/b... File components/bookmarks/browser/bookmark_utils_unittest.cc (right): https://codereview.chromium.org/1892483005/diff/160001/components/bookmarks/b... components/bookmarks/browser/bookmark_utils_unittest.cc:89: #if defined(USE_X11) To be safe, guard all these with both USE_GLIB and USE_X11?
[ Please update the CL description to briefly mention the intricacies of CurrentTime etc. ]
Description was changed from ========== SelectionOwner: add support for TIMESTAMP target BUG=395077 ========== to ========== SelectionOwner: add support for TIMESTAMP target Even though XSetSelectionOwner() has a Time argument, there's no actual way to retrieve the timestamp associated with the selection from the X server itself. Instead, the X11 selection protocol requires selections to provide a TIMESTAMP target. Unfortunately, there's also no good way to get the BUG=395077 ==========
Description was changed from ========== SelectionOwner: add support for TIMESTAMP target Even though XSetSelectionOwner() has a Time argument, there's no actual way to retrieve the timestamp associated with the selection from the X server itself. Instead, the X11 selection protocol requires selections to provide a TIMESTAMP target. Unfortunately, there's also no good way to get the BUG=395077 ========== to ========== SelectionOwner: add support for TIMESTAMP target Even though XSetSelectionOwner() has a Time argument, there's no way to retrieve the timestamp associated with the selection from the X server itself. Instead, the X11 selection protocol requires selections to provide a TIMESTAMP target. Unfortunately, there's also no good way to get the current timestamp from the X server: one way is to make a no-op property change and observe the resulting PropertyNotify event. This is the what GTK does, but it both blocks and performs out-of-order event dispatching. In Chromium, we have control over our event source, so we implement an alternate strategy: we record the server time in X events with a time field. BUG=395077 ==========
Description was changed from ========== SelectionOwner: add support for TIMESTAMP target Even though XSetSelectionOwner() has a Time argument, there's no way to retrieve the timestamp associated with the selection from the X server itself. Instead, the X11 selection protocol requires selections to provide a TIMESTAMP target. Unfortunately, there's also no good way to get the current timestamp from the X server: one way is to make a no-op property change and observe the resulting PropertyNotify event. This is the what GTK does, but it both blocks and performs out-of-order event dispatching. In Chromium, we have control over our event source, so we implement an alternate strategy: we record the server time in X events with a time field. BUG=395077 ========== to ========== SelectionOwner: add support for TIMESTAMP target Even though XSetSelectionOwner() has a Time argument, there's no way to retrieve the timestamp associated with the selection from the X server itself. Instead, the X11 selection protocol requires selections to provide a TIMESTAMP target. Unfortunately, there's also no good way to get the current timestamp from the X server: one way is to make a no-op property change and observe the resulting PropertyNotify event. This is the what GTK does, but it both blocks and performs out-of-order event dispatching. In Chromium, we control our event source, so we implement an alternate strategy instead: we record the server time seen in X events with a time field and use the last seen value when taking ownership of the X selection. Of course, since this is X, the server time observer has to be careful to ignore instances where time == CurrentTime (explicitly set by some programs to indicate an X event is synthesized) and where time goes backwards (since events can be reordered). Finally, even though we BUG=395077 ==========
Description was changed from ========== SelectionOwner: add support for TIMESTAMP target Even though XSetSelectionOwner() has a Time argument, there's no way to retrieve the timestamp associated with the selection from the X server itself. Instead, the X11 selection protocol requires selections to provide a TIMESTAMP target. Unfortunately, there's also no good way to get the current timestamp from the X server: one way is to make a no-op property change and observe the resulting PropertyNotify event. This is the what GTK does, but it both blocks and performs out-of-order event dispatching. In Chromium, we control our event source, so we implement an alternate strategy instead: we record the server time seen in X events with a time field and use the last seen value when taking ownership of the X selection. Of course, since this is X, the server time observer has to be careful to ignore instances where time == CurrentTime (explicitly set by some programs to indicate an X event is synthesized) and where time goes backwards (since events can be reordered). Finally, even though we BUG=395077 ========== to ========== SelectionOwner: add support for TIMESTAMP target Even though XSetSelectionOwner() has a Time argument, there's no way to retrieve the timestamp associated with the selection from the X server itself. Instead, the X11 selection protocol requires selections to provide a TIMESTAMP target. Unfortunately, there's also no good way to get the current timestamp from the X server: one way is to make a no-op property change and observe the resulting PropertyNotify event. This is the what GTK does, but it both blocks and performs out-of-order event dispatching. In Chromium, we control our event source, so we implement an alternate strategy instead: we record the server time seen in X events with a time field and use the last seen value when taking ownership of the X selection. Of course, since this is X, the server time observer has to be careful to ignore instances where time == CurrentTime (explicitly set by some programs to indicate an X event is synthesized) and where time goes backwards (since events can be reordered). Note that despite all this logic to sanitize X server time, actually using the last seen time when calling XSetSelectionOwner() randomly causes tests to fail… so we still use CurrentTime there. ¯\_(ツ)_/¯ BUG=395077 ==========
Description was changed from ========== SelectionOwner: add support for TIMESTAMP target Even though XSetSelectionOwner() has a Time argument, there's no way to retrieve the timestamp associated with the selection from the X server itself. Instead, the X11 selection protocol requires selections to provide a TIMESTAMP target. Unfortunately, there's also no good way to get the current timestamp from the X server: one way is to make a no-op property change and observe the resulting PropertyNotify event. This is the what GTK does, but it both blocks and performs out-of-order event dispatching. In Chromium, we control our event source, so we implement an alternate strategy instead: we record the server time seen in X events with a time field and use the last seen value when taking ownership of the X selection. Of course, since this is X, the server time observer has to be careful to ignore instances where time == CurrentTime (explicitly set by some programs to indicate an X event is synthesized) and where time goes backwards (since events can be reordered). Note that despite all this logic to sanitize X server time, actually using the last seen time when calling XSetSelectionOwner() randomly causes tests to fail… so we still use CurrentTime there. ¯\_(ツ)_/¯ BUG=395077 ========== to ========== SelectionOwner: add support for TIMESTAMP target Even though XSetSelectionOwner() has a Time argument, there's no way to retrieve the timestamp associated with the selection from the X server itself. Instead, the X11 selection protocol requires selections to provide a TIMESTAMP target. Unfortunately, there's also no good way to get the current timestamp from the X server: one way is to make a no-op property change and observe the resulting PropertyNotify event. This is the what GTK does, but it both blocks and performs out-of-order event dispatching. In Chromium, we control our event source, so we implement an alternate strategy instead: we record the server time seen in X events with a time field and use the last seen value when taking ownership of the X selection. Of course, since this is X, the server time observer has to be careful to ignore instances where time == CurrentTime (explicitly set by some programs to indicate an X event is synthesized) and where time goes backwards (since events can be reordered). Note that despite all this logic to sanitize X server time, actually using the last seen time when calling XSetSelectionOwner() doesn't actually seem to be good enough. Random omnibox tests still fail, so we still pass CurrentTime to XSetSelectionOwner(). ¯\_(ツ)_/¯ BUG=395077 ==========
Description was changed from ========== SelectionOwner: add support for TIMESTAMP target Even though XSetSelectionOwner() has a Time argument, there's no way to retrieve the timestamp associated with the selection from the X server itself. Instead, the X11 selection protocol requires selections to provide a TIMESTAMP target. Unfortunately, there's also no good way to get the current timestamp from the X server: one way is to make a no-op property change and observe the resulting PropertyNotify event. This is the what GTK does, but it both blocks and performs out-of-order event dispatching. In Chromium, we control our event source, so we implement an alternate strategy instead: we record the server time seen in X events with a time field and use the last seen value when taking ownership of the X selection. Of course, since this is X, the server time observer has to be careful to ignore instances where time == CurrentTime (explicitly set by some programs to indicate an X event is synthesized) and where time goes backwards (since events can be reordered). Note that despite all this logic to sanitize X server time, actually using the last seen time when calling XSetSelectionOwner() doesn't actually seem to be good enough. Random omnibox tests still fail, so we still pass CurrentTime to XSetSelectionOwner(). ¯\_(ツ)_/¯ BUG=395077 ========== to ========== SelectionOwner: add support for TIMESTAMP target Even though XSetSelectionOwner() has a Time argument, there's no way to retrieve the timestamp associated with the selection from the X server itself. Instead, the X11 selection protocol requires selections to provide a TIMESTAMP target. Unfortunately, there's also no good way to get the current timestamp from the X server: one way is to make a no-op property change and observe the resulting PropertyNotify event. This is the what GTK does, but it both blocks and performs out-of-order event dispatching. In Chromium, we control our event source, so we implement an alternate strategy instead: we record the server time seen in X events with a time field and use the last seen value when taking ownership of the X selection. Of course, since this is X, the server time observer has to be careful to ignore instances where time == CurrentTime (explicitly set by some programs to indicate an X event is synthesized) and where time goes backwards (since events can be reordered). Note that despite all this logic to sanitize X server time, actually using the last seen time when calling XSetSelectionOwner() doesn't actually seem to be good enough. Random omnibox tests still fail in that case, so we still pass CurrentTime to XSetSelectionOwner() to increase the chances of success. ¯\_(ツ)_/¯ BUG=395077 ==========
The CQ bit was checked by dcheng@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from erg@chromium.org, sadrul@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/1892483005/#ps180001 (title: "USE_GLIB")
Also updated the CL description with my sad tale. https://codereview.chromium.org/1892483005/diff/160001/components/bookmarks/b... File components/bookmarks/browser/bookmark_utils_unittest.cc (right): https://codereview.chromium.org/1892483005/diff/160001/components/bookmarks/b... components/bookmarks/browser/bookmark_utils_unittest.cc:89: #if defined(USE_X11) On 2016/04/22 at 17:17:25, sadrul wrote: > To be safe, guard all these with both USE_GLIB and USE_X11? Done.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1892483005/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1892483005/180001
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== SelectionOwner: add support for TIMESTAMP target Even though XSetSelectionOwner() has a Time argument, there's no way to retrieve the timestamp associated with the selection from the X server itself. Instead, the X11 selection protocol requires selections to provide a TIMESTAMP target. Unfortunately, there's also no good way to get the current timestamp from the X server: one way is to make a no-op property change and observe the resulting PropertyNotify event. This is the what GTK does, but it both blocks and performs out-of-order event dispatching. In Chromium, we control our event source, so we implement an alternate strategy instead: we record the server time seen in X events with a time field and use the last seen value when taking ownership of the X selection. Of course, since this is X, the server time observer has to be careful to ignore instances where time == CurrentTime (explicitly set by some programs to indicate an X event is synthesized) and where time goes backwards (since events can be reordered). Note that despite all this logic to sanitize X server time, actually using the last seen time when calling XSetSelectionOwner() doesn't actually seem to be good enough. Random omnibox tests still fail in that case, so we still pass CurrentTime to XSetSelectionOwner() to increase the chances of success. ¯\_(ツ)_/¯ BUG=395077 ========== to ========== SelectionOwner: add support for TIMESTAMP target Even though XSetSelectionOwner() has a Time argument, there's no way to retrieve the timestamp associated with the selection from the X server itself. Instead, the X11 selection protocol requires selections to provide a TIMESTAMP target. Unfortunately, there's also no good way to get the current timestamp from the X server: one way is to make a no-op property change and observe the resulting PropertyNotify event. This is the what GTK does, but it both blocks and performs out-of-order event dispatching. In Chromium, we control our event source, so we implement an alternate strategy instead: we record the server time seen in X events with a time field and use the last seen value when taking ownership of the X selection. Of course, since this is X, the server time observer has to be careful to ignore instances where time == CurrentTime (explicitly set by some programs to indicate an X event is synthesized) and where time goes backwards (since events can be reordered). Note that despite all this logic to sanitize X server time, actually using the last seen time when calling XSetSelectionOwner() doesn't actually seem to be good enough. Random omnibox tests still fail in that case, so we still pass CurrentTime to XSetSelectionOwner() to increase the chances of success. ¯\_(ツ)_/¯ BUG=395077 Committed: https://crrev.com/5c42da4d3ec29ffaf0bb9d3fbd33a55f75249781 Cr-Commit-Position: refs/heads/master@{#389208} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/5c42da4d3ec29ffaf0bb9d3fbd33a55f75249781 Cr-Commit-Position: refs/heads/master@{#389208} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
