Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
817983002:
ozone: xkb: Load keymaps on worker thread & cache them (Closed)
|
Created:
6 years ago by FengYuan Modified:
5 years, 11 months ago CC:
chromium-reviews, kalyank, tdresser+watch_chromium.org, jdduke+watch_chromium.org, ozone-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionozone: xkb: Load keymaps on worker thread & cache them
1. Free xkb keymaps in the destructor.
2. Moves kemap load into another worker thread.
3. Adds keymap caching.
BUG=430194
Committed: https://crrev.com/f0b38f78f146cedf336ac9ad0236f681833c7263
Cr-Commit-Position: refs/heads/master@{#309928}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #
Total comments: 8
Patch Set 5 : #Patch Set 6 : #Patch Set 7 : #Patch Set 8 : #Patch Set 9 : Rebase #
Total comments: 10
Patch Set 10 : #Patch Set 11 : #Patch Set 12 : #Patch Set 13 : #Patch Set 14 : Adds unittests #
Total comments: 20
Patch Set 15 : Figured out Michael's comments #
Total comments: 15
Patch Set 16 : #
Total comments: 2
Patch Set 17 : #
Total comments: 1
Messages
Total messages: 28 (3 generated)
fengyuan@chromium.org changed reviewers: + shuchen@chromium.org
https://codereview.chromium.org/817983002/diff/60001/ui/events/ozone/layout/x... File ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.cc (right): https://codereview.chromium.org/817983002/diff/60001/ui/events/ozone/layout/x... ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.cc:655: base::Unretained(this), base::Owned(this) https://codereview.chromium.org/817983002/diff/60001/ui/events/ozone/layout/x... ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.cc:662: const std::string& layout_name, xkb_rule_names* names) { You need to delete names in this method. https://codereview.chromium.org/817983002/diff/60001/ui/events/ozone/layout/x... ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.cc:669: layout_engine_->xkb_keymaps_[layout_name] = keymap; Please make sure these operations are executed in UI thread, otherwise, you need to make sure it is thread safe, because it is possible for 2 threads to execute operations on layout_engine_ at the same time. I think using PostTaskAndReply may be better. https://codereview.chromium.org/817983002/diff/60001/ui/events/ozone/layout/x... ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.cc:711: XkbKeymapLoader* keymap_loader = new XkbKeymapLoader(this); nowhere to delete keymap_loader? Suggest to make it as a member variable.
fengyuan@chromium.org changed reviewers: + kpschoedel@chromium.org, spang@chromium.org
https://codereview.chromium.org/817983002/diff/60001/ui/events/ozone/layout/x... File ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.cc (right): https://codereview.chromium.org/817983002/diff/60001/ui/events/ozone/layout/x... ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.cc:655: base::Unretained(this), On 2014/12/23 08:46:49, Shu Chen wrote: > base::Owned(this) Done. https://codereview.chromium.org/817983002/diff/60001/ui/events/ozone/layout/x... ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.cc:662: const std::string& layout_name, xkb_rule_names* names) { On 2014/12/23 08:46:49, Shu Chen wrote: > You need to delete names in this method. Done. https://codereview.chromium.org/817983002/diff/60001/ui/events/ozone/layout/x... ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.cc:669: layout_engine_->xkb_keymaps_[layout_name] = keymap; On 2014/12/23 08:46:49, Shu Chen wrote: > Please make sure these operations are executed in UI thread, otherwise, you need > to make sure it is thread safe, because it is possible for 2 threads to execute > operations on layout_engine_ at the same time. > > I think using PostTaskAndReply may be better. These operations are in the worker thread, and reply is not necessary. https://codereview.chromium.org/817983002/diff/60001/ui/events/ozone/layout/x... ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.cc:711: XkbKeymapLoader* keymap_loader = new XkbKeymapLoader(this); On 2014/12/23 08:46:49, Shu Chen wrote: > nowhere to delete keymap_loader? Suggest to make it as a member variable. Done.
https://codereview.chromium.org/817983002/diff/160001/ui/events/ozone/layout/... File ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.cc (right): https://codereview.chromium.org/817983002/diff/160001/ui/events/ozone/layout/... ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.cc:840: if (keymap) { Please move the below block to UI thread via PostTaskAndReply. https://codereview.chromium.org/817983002/diff/160001/ui/events/ozone/layout/... ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.cc:845: layout_engine_->SetCurrentLayoutByName("us"); It'd be better to do: if (!keymap) keymap = layout_engine_->xkb_keymaps_["us"]; layout_engine_->SetKeymap(keymap); https://codereview.chromium.org/817983002/diff/160001/ui/events/ozone/layout/... ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.cc:875: delete keymap_loader_; Please make keymap_loader_ as scoped_ptr.
https://codereview.chromium.org/817983002/diff/160001/ui/events/ozone/layout/... File ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.h (right): https://codereview.chromium.org/817983002/diff/160001/ui/events/ozone/layout/... ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.h:94: class XkbKeymapLoader : public base::RefCountedThreadSafe<XkbKeymapLoader> { Usually we put inner class definition as the first section in class' definition. https://codereview.chromium.org/817983002/diff/160001/ui/events/ozone/layout/... ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.h:110: XkbKeymapLoader* keymap_loader_; Use scoped_ptr
PTAL. https://codereview.chromium.org/817983002/diff/160001/ui/events/ozone/layout/... File ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.cc (right): https://codereview.chromium.org/817983002/diff/160001/ui/events/ozone/layout/... ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.cc:840: if (keymap) { On 2014/12/24 02:18:45, Shu Chen wrote: > Please move the below block to UI thread via PostTaskAndReply. Done. https://codereview.chromium.org/817983002/diff/160001/ui/events/ozone/layout/... ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.cc:845: layout_engine_->SetCurrentLayoutByName("us"); On 2014/12/24 02:18:45, Shu Chen wrote: > It'd be better to do: > > if (!keymap) > keymap = layout_engine_->xkb_keymaps_["us"]; > layout_engine_->SetKeymap(keymap); > Done. https://codereview.chromium.org/817983002/diff/160001/ui/events/ozone/layout/... ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.cc:875: delete keymap_loader_; On 2014/12/24 02:18:45, Shu Chen wrote: > Please make keymap_loader_ as scoped_ptr. Done. https://codereview.chromium.org/817983002/diff/160001/ui/events/ozone/layout/... File ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.h (right): https://codereview.chromium.org/817983002/diff/160001/ui/events/ozone/layout/... ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.h:94: class XkbKeymapLoader : public base::RefCountedThreadSafe<XkbKeymapLoader> { On 2014/12/24 02:24:21, Shu Chen wrote: > Usually we put inner class definition as the first section in class' definition. Done. https://codereview.chromium.org/817983002/diff/160001/ui/events/ozone/layout/... ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.h:110: XkbKeymapLoader* keymap_loader_; On 2014/12/24 02:24:21, Shu Chen wrote: > Use scoped_ptr Done.
lgtm Please update the CL description to reflect the changes.
We may want to look into shipping these as a resource bundle. https://codereview.chromium.org/817983002/diff/260001/ui/events/ozone/layout/... File ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.cc (right): https://codereview.chromium.org/817983002/diff/260001/ui/events/ozone/layout/... ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.cc:31: const char* us_keymap = I'm not sure we want this here. We'll always ship the keymap - if it's missing or cannot be loaded that's a fatal error. Why is it necessary to have a fallback? https://codereview.chromium.org/817983002/diff/260001/ui/events/ozone/layout/... ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.cc:825: for(;it != xkb_keymaps_.end(); ++it) { Wrong whitespace, please git cl format. https://codereview.chromium.org/817983002/diff/260001/ui/events/ozone/layout/... ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.cc:838: void loadKeymap(const std::string& layout_name, LoadKeymap https://codereview.chromium.org/817983002/diff/260001/ui/events/ozone/layout/... ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.cc:849: base::Unretained(keymap))); Using base::Unretained() leaks the keymap whenever the weak pointer is invalidated. You want base::Passed(). https://codereview.chromium.org/817983002/diff/260001/ui/events/ozone/layout/... ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.cc:858: SetKeymap(keymap); if (keymap) { SetKeyMap(keymap) return true; } Then move the rest of the function outside the else {}. https://codereview.chromium.org/817983002/diff/260001/ui/events/ozone/layout/... ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.cc:860: if (xkb_keymaps_.find(layout_name) != std::map::end) I think you meant if (xkb_keymaps_.find(layout_name) != xkb_keymaps_.end()) or (shorter) if (xkb_keymaps_.count(layout_name)) https://codereview.chromium.org/817983002/diff/260001/ui/events/ozone/layout/... ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.cc:876: ui_task_runner_, Just write base::ThreadTaskRunnerHandle::Get() here. Doesn't look like you need ui_task_runner_. https://codereview.chromium.org/817983002/diff/260001/ui/events/ozone/layout/... ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.cc:914: xkb_keymaps_[layout_name] = keymap; Is it OK caching these indefinitely in a map? I guess it probably is, but at the same time it seems having just a handful in a vector would be less overkill. https://codereview.chromium.org/817983002/diff/260001/ui/events/ozone/layout/... ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.cc:915: if (layout_name.compare(current_layout_name_) == 0) if (layout_name == current_layout_name_)
Thanks for your review, PTAL :) https://codereview.chromium.org/817983002/diff/260001/ui/events/ozone/layout/... File ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.cc (right): https://codereview.chromium.org/817983002/diff/260001/ui/events/ozone/layout/... ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.cc:31: const char* us_keymap = On 2015/01/02 20:16:35, spang wrote: > I'm not sure we want this here. We'll always ship the keymap - if it's missing > or cannot be loaded that's a fatal error. > > Why is it necessary to have a fallback? I reintroduced the fallback here because of issue: https://code.google.com/p/chromium/issues/detail?id=444062 Since it is fixed now, I think it's safe to remove the fallback. @shuchen, how about your idea? https://codereview.chromium.org/817983002/diff/260001/ui/events/ozone/layout/... ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.cc:825: for(;it != xkb_keymaps_.end(); ++it) { On 2015/01/02 20:16:34, spang wrote: > Wrong whitespace, please git cl format. Done. https://codereview.chromium.org/817983002/diff/260001/ui/events/ozone/layout/... ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.cc:838: void loadKeymap(const std::string& layout_name, On 2015/01/02 20:16:35, spang wrote: > LoadKeymap Done. https://codereview.chromium.org/817983002/diff/260001/ui/events/ozone/layout/... ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.cc:849: base::Unretained(keymap))); On 2015/01/02 20:16:35, spang wrote: > Using base::Unretained() leaks the keymap whenever the weak pointer is > invalidated. You want base::Passed(). Done. https://codereview.chromium.org/817983002/diff/260001/ui/events/ozone/layout/... ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.cc:858: SetKeymap(keymap); On 2015/01/02 20:16:35, spang wrote: > if (keymap) { > SetKeyMap(keymap) > return true; > } > > Then move the rest of the function outside the else {}. Done. https://codereview.chromium.org/817983002/diff/260001/ui/events/ozone/layout/... ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.cc:860: if (xkb_keymaps_.find(layout_name) != std::map::end) On 2015/01/02 20:16:35, spang wrote: > I think you meant > > if (xkb_keymaps_.find(layout_name) != xkb_keymaps_.end()) > > or (shorter) > > if (xkb_keymaps_.count(layout_name)) Acknowledged. https://codereview.chromium.org/817983002/diff/260001/ui/events/ozone/layout/... ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.cc:876: ui_task_runner_, On 2015/01/02 20:16:35, spang wrote: > Just write > > base::ThreadTaskRunnerHandle::Get() > > here. Doesn't look like you need ui_task_runner_. Done. https://codereview.chromium.org/817983002/diff/260001/ui/events/ozone/layout/... ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.cc:914: xkb_keymaps_[layout_name] = keymap; On 2015/01/02 20:16:34, spang wrote: > Is it OK caching these indefinitely in a map? I guess it probably is, but at the > same time it seems having just a handful in a vector would be less overkill. Done. https://codereview.chromium.org/817983002/diff/260001/ui/events/ozone/layout/... ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.cc:915: if (layout_name.compare(current_layout_name_) == 0) On 2015/01/02 20:16:35, spang wrote: > if (layout_name == current_layout_name_) Done.
Sorry, found the latest patch could be further improved. So not lgtm yet. https://codereview.chromium.org/817983002/diff/260001/ui/events/ozone/layout/... File ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.cc (right): https://codereview.chromium.org/817983002/diff/260001/ui/events/ozone/layout/... ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.cc:31: const char* us_keymap = On 2015/01/03 17:57:04, FengYuan wrote: > On 2015/01/02 20:16:35, spang wrote: > > I'm not sure we want this here. We'll always ship the keymap - if it's missing > > or cannot be loaded that's a fatal error. > > > > Why is it necessary to have a fallback? > > I reintroduced the fallback here because of issue: > https://code.google.com/p/chromium/issues/detail?id=444062 > > Since it is fixed now, I think it's safe to remove the fallback. @shuchen, how > about your idea? Agreed to remove this. https://codereview.chromium.org/817983002/diff/140002/ui/events/ozone/layout/... File ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.cc (right): https://codereview.chromium.org/817983002/diff/140002/ui/events/ozone/layout/... ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.cc:27: LoadKeymapCallback; Please move this to the anonymous namespace below. https://codereview.chromium.org/817983002/diff/140002/ui/events/ozone/layout/... ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.cc:670: } Please move this global function to the anonymous namespace above. https://codereview.chromium.org/817983002/diff/140002/ui/events/ozone/layout/... ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.cc:676: for (const auto& entry : xkb_keymaps_) { Not sure whether the Chromium building systems are fully supporting C++11. spang@, do you have any ideas? https://codereview.chromium.org/817983002/diff/140002/ui/events/ozone/layout/... ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.cc:724: if (keymap) { keymap.get() https://codereview.chromium.org/817983002/diff/140002/ui/events/ozone/layout/... ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.cc:726: xkb_keymaps_.push_back(entry); There is risk of inserting duplicated entries here. Suggest to add some logic in SetCurrentLayoutByName method to avoid duplicated loading. https://codereview.chromium.org/817983002/diff/140002/ui/events/ozone/layout/... ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.cc:729: } Suggest to do an error log here. else { LOG(ERROR) << .... }
https://codereview.chromium.org/817983002/diff/260001/ui/events/ozone/layout/... File ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.cc (right): https://codereview.chromium.org/817983002/diff/260001/ui/events/ozone/layout/... ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.cc:31: const char* us_keymap = On 2015/01/04 01:43:16, Shu Chen wrote: > On 2015/01/03 17:57:04, FengYuan wrote: > > On 2015/01/02 20:16:35, spang wrote: > > > I'm not sure we want this here. We'll always ship the keymap - if it's > missing > > > or cannot be loaded that's a fatal error. > > > > > > Why is it necessary to have a fallback? > > > > I reintroduced the fallback here because of issue: > > https://code.google.com/p/chromium/issues/detail?id=444062 > > > > Since it is fixed now, I think it's safe to remove the fallback. @shuchen, how > > about your idea? > > Agreed to remove this. Done. https://codereview.chromium.org/817983002/diff/140002/ui/events/ozone/layout/... File ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.cc (right): https://codereview.chromium.org/817983002/diff/140002/ui/events/ozone/layout/... ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.cc:27: LoadKeymapCallback; On 2015/01/04 01:43:16, Shu Chen wrote: > Please move this to the anonymous namespace below. Done. https://codereview.chromium.org/817983002/diff/140002/ui/events/ozone/layout/... ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.cc:670: } On 2015/01/04 01:43:17, Shu Chen wrote: > Please move this global function to the anonymous namespace above. Done. https://codereview.chromium.org/817983002/diff/140002/ui/events/ozone/layout/... ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.cc:676: for (const auto& entry : xkb_keymaps_) { On 2015/01/04 01:43:16, Shu Chen wrote: > Not sure whether the Chromium building systems are fully supporting C++11. > > spang@, do you have any ideas? It is existing in the above codes, so it should be fine. https://codereview.chromium.org/817983002/diff/140002/ui/events/ozone/layout/... ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.cc:724: if (keymap) { On 2015/01/04 01:43:17, Shu Chen wrote: > keymap.get() not necessary, see same usage in XkbLookup https://codereview.chromium.org/817983002/diff/140002/ui/events/ozone/layout/... ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.cc:726: xkb_keymaps_.push_back(entry); On 2015/01/04 01:43:16, Shu Chen wrote: > There is risk of inserting duplicated entries here. > > Suggest to add some logic in SetCurrentLayoutByName method to avoid duplicated > loading. yep, duplicate entries doesn't do harm to the logic. https://codereview.chromium.org/817983002/diff/140002/ui/events/ozone/layout/... ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.cc:729: } On 2015/01/04 01:43:17, Shu Chen wrote: > Suggest to do an error log here. > > else { > LOG(ERROR) << .... > } Done.
https://codereview.chromium.org/817983002/diff/140002/ui/events/ozone/layout/... File ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.cc (right): https://codereview.chromium.org/817983002/diff/140002/ui/events/ozone/layout/... ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.cc:726: xkb_keymaps_.push_back(entry); On 2015/01/04 02:25:22, FengYuan wrote: > On 2015/01/04 01:43:16, Shu Chen wrote: > > There is risk of inserting duplicated entries here. > > > > Suggest to add some logic in SetCurrentLayoutByName method to avoid duplicated > > loading. > > yep, duplicate entries doesn't do harm to the logic. Are you going to create further CLs to solve the dup entries?
https://codereview.chromium.org/817983002/diff/290001/ui/events/ozone/layout/... File ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.cc (right): https://codereview.chromium.org/817983002/diff/290001/ui/events/ozone/layout/... ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.cc:725: XkbKeymapEntry entry = {layout_name, keymap.get()}; This is potentially a crash issue, because the addref is missing on keymap. Please do: XkbKeymapEntry entry = {layout_name, xkb_keymap_ref(keymap.get())};
Thanks for your review, PTAL :) https://codereview.chromium.org/817983002/diff/140002/ui/events/ozone/layout/... File ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.cc (right): https://codereview.chromium.org/817983002/diff/140002/ui/events/ozone/layout/... ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.cc:726: xkb_keymaps_.push_back(entry); On 2015/01/04 02:30:59, Shu Chen wrote: > On 2015/01/04 02:25:22, FengYuan wrote: > > On 2015/01/04 01:43:16, Shu Chen wrote: > > > There is risk of inserting duplicated entries here. > > > > > > Suggest to add some logic in SetCurrentLayoutByName method to avoid > duplicated > > > loading. > > > > yep, duplicate entries doesn't do harm to the logic. > > Are you going to create further CLs to solve the dup entries? Yep, I will solve the duplicate loading in IMF side. https://codereview.chromium.org/817983002/diff/290001/ui/events/ozone/layout/... File ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.cc (right): https://codereview.chromium.org/817983002/diff/290001/ui/events/ozone/layout/... ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.cc:725: XkbKeymapEntry entry = {layout_name, keymap.get()}; On 2015/01/04 02:43:39, Shu Chen wrote: > This is potentially a crash issue, because the addref is missing on keymap. > > Please do: > XkbKeymapEntry entry = {layout_name, xkb_keymap_ref(keymap.get())}; > > Done.
lgtm
nit: the #4 of CL description should be removed.
The CQ bit was checked by spang@chromium.org
lgtm
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/817983002/310001
https://codereview.chromium.org/817983002/diff/140002/ui/events/ozone/layout/... File ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.cc (right): https://codereview.chromium.org/817983002/diff/140002/ui/events/ozone/layout/... ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.cc:676: for (const auto& entry : xkb_keymaps_) { On 2015/01/04 02:25:22, FengYuan wrote: > On 2015/01/04 01:43:16, Shu Chen wrote: > > Not sure whether the Chromium building systems are fully supporting C++11. > > > > spang@, do you have any ideas? > > It is existing in the above codes, so it should be fine. Correct. Here's the full reference: https://chromium-cpp.appspot.com/
Message was sent while issue was closed.
Committed patchset #17 (id:310001)
Message was sent while issue was closed.
Patchset 17 (id:??) landed as https://crrev.com/f0b38f78f146cedf336ac9ad0236f681833c7263 Cr-Commit-Position: refs/heads/master@{#309928}
Message was sent while issue was closed.
https://codereview.chromium.org/817983002/diff/310001/ui/events/ozone/layout/... File ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.cc (right): https://codereview.chromium.org/817983002/diff/310001/ui/events/ozone/layout/... ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.cc:688: base::Unretained(xkb_context_.get()), ..oh dear I didn't look at this closely enough. It doesn't look like xkb_context is thread safe. Even if it was, posting it to to the WorkerPool inside base::Unretained is definitely not safe.
Message was sent while issue was closed.
A revert of this CL (patchset #17 id:310001) has been created in https://codereview.chromium.org/823633003/ by spang@chromium.org. The reason for reverting is: Adds thread-unsafe code..
Message was sent while issue was closed.
On 2015/01/06 17:35:50, spang wrote: > https://codereview.chromium.org/817983002/diff/310001/ui/events/ozone/layout/... > File ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.cc (right): > > https://codereview.chromium.org/817983002/diff/310001/ui/events/ozone/layout/... > ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.cc:688: > base::Unretained(xkb_context_.get()), > ..oh dear > > I didn't look at this closely enough. It doesn't look like xkb_context is thread > safe. Even if it was, posting it to to the WorkerPool inside base::Unretained is > definitely not safe. Maybe the way to go (based on the comments in <xkbcommon/xkbcommon.h>) is: - loading thread creates its own xkb_context and loads the files - loading thread calls xkb_keymap_get_as_string() and returns the string - IO thread calls xkb_keymap_new_from_string() |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
