Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
194313003:
Removed the unnecessary TODO (Closed)
|
Created:
6 years, 9 months ago by suyash Modified:
6 years, 9 months ago CC:
chromium-reviews, yusukes+watch_chromium.org, yukishiino+watch_chromium.org, tfarina, penghuang+watch_chromium.org, nona+watch_chromium.org, James Su Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionRemoved the unnecessary TODO
There was a TODO to move the typedefs from input_method.h to ime_constants.h. As per the discussion on the previous patch set,removed the TODO and kept the typedefs in their original location.
This patch is for doing the same.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=256614
Patch Set 1 #Patch Set 2 : Changes in accordance to lint and gave correct path to ime_constants.h #Patch Set 3 : #
Messages
Total messages: 25 (0 generated)
@nico @yukawa kindly review this patch.
On 2014/03/11 08:37:15, suyash.s wrote: > @nico @yukawa kindly review this patch. Thank you for taking a look at that TODO. Well, I'm now wondering if it's really worth doing or not. Even though that that #ifdef doesn't look great, the fact that the actual usage of NativeEventResult is limited to IME classes makes me hesitate to generalize it. So could you keep the typedef location as it is? Regarding the TODO comment, feel free to remove it if you want. Thank you again for taking care of the cleanup.
On 2014/03/11 11:02:04, yukawa wrote: > On 2014/03/11 08:37:15, suyash.s wrote: > > @nico @yukawa kindly review this patch. > > Thank you for taking a look at that TODO. Well, I'm now wondering if it's > really worth doing or not. Even though that that #ifdef doesn't look great, the > fact that the actual usage of NativeEventResult is limited to IME classes makes > me hesitate to generalize it. So could you keep the typedef location as it is? > Regarding the TODO comment, feel free to remove it if you want. > > Thank you again for taking care of the cleanup. @yukawa thanks for taking a look at the patch! My initial thought was the same. #ifdef didnt look great. I will keep the typedef location as it is and remove the TODO (as it is not required as per you) and upload a new patch set soon.
Uploaded the patch to remove the TODO.
Thanks! LGTM
The CQ bit was checked by suyash.s@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/suyash.s@samsung.com/194313003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel
lgtm stamp
The CQ bit was checked by suyash.s@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/suyash.s@samsung.com/194313003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel
The CQ bit was checked by suyash.s@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/suyash.s@samsung.com/194313003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel
The CQ bit was checked by suyash.s@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/suyash.s@samsung.com/194313003/40001
The CQ bit was unchecked by suyash.s@samsung.com
The CQ bit was checked by suyash.s@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/suyash.s@samsung.com/194313003/40001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/suyash.s@samsung.com/194313003/40001
Message was sent while issue was closed.
Change committed as 256614 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
