Wrong use IDS_ string identifiers as IDC_ commands.
Reported by
mer...@yandex-team.ru,
Jun 21 2017
|
|||||
Issue descriptionUserAgent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/57.0.2987.137 YaBrowser/17.4.1.919 Yowser/2.5 Safari/537.36 Steps to reproduce the problem: Some string IDs are used as command IDs. Namely, IDS_APP_UNDO, IDS_APP_CUT, IDS_APP_PASTE, IDS_APP_DELETE, IDS_APP_SELECT_ALL, IDS_PASTE_AND_GO. This is bad practice, because auto-generated integer values for string ids may cross with manually defined command ids. What is the expected behavior? Expected to use pairs - IDC_APP_PASTE for command id and IDS_APP_PASTER for human-readable string of its menu item, and so on. What went wrong? In fact, it works yet. Just a errorprone style of programming. Did this work before? N/A Chrome version: 60.0.3108.0 Channel: dev OS Version: 6.1 (Windows 7, Windows Server 2008 R2) Flash Version: Shockwave Flash 26.0 r0 Files involved: src\ui\views\controls\textfield\textfield.cc ui\views\touchui\touch_selection_menu_runner_views.cc content\browser\renderer_host\input\touch_selection_controller_client_aura.cc src\chrome\browser\ui\views\omnibox\omnibox_view_views.cc
,
Jun 30 2017
This issue seems to be out of TE-scope. Hence, adding label TE-NeedsTriageHelp for further investigation. Thanks...!!
,
Jul 6 2017
pkasting: do you know why we don't use IDC_ in omnibox_view_views.cc? There's a suggestive but cryptic comment in that file: "We use IDC_ for command id here while the underlying textfield is using IDS_ for all its command ids. This is because views cannot depend on IDC_ for now." added in https://chromiumcodereview.appspot.com/9250016/.
,
Jul 6 2017
,
Jul 6 2017
@0: I don't know whether this pattern is actually error-prone, because both IDC_ and IDS_ values are restricted to specific ranges, and I'm not sure those ranges overlap (I think they don't, not sure). @3: Things like IDC_CUT are defined in chrome/app/chrome_command_ids.h, which ui/views/controls/textfield/textfield.h can't depend on (layering violation). If you wanted to use these commands there, you'd need to hoist them to a header that could be #included from both sides. I suspect no one has done this because it doesn't seem like a win.
,
Jul 7 2017
Closing based on our belief that the ranges don't overlap. Feel free to re-open with evidence to the contrary. |
|||||
►
Sign in to add a comment |
|||||
Comment 1 by ranjitkan@chromium.org
, Jun 29 2017