Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
438193003:
Updating MakeMenuItemStringsFor() to include custom items if provided by blink. (Closed)
DescriptionUpdating MakeMenuItemStringsFor() to include custom items if provided by blink.
BUG=87553
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=287527
Patch Set 1 #
Total comments: 2
Patch Set 2 : Added submenu and menu depth identifier #
Total comments: 14
Patch Set 3 : Fixed #
Total comments: 8
Patch Set 4 : #
Total comments: 2
Patch Set 5 : #Messages
Total messages: 12 (0 generated)
This will be used for layout testing custom context menu https://codereview.chromium.org/243403006/. PTAL.
https://codereview.chromium.org/438193003/diff/1/content/shell/renderer/test_... File content/shell/renderer/test_runner/event_sender.cc (right): https://codereview.chromium.org/438193003/diff/1/content/shell/renderer/test_... content/shell/renderer/test_runner/event_sender.cc:136: PopulateCustomItems(customItems[i].subMenuItems, strings); Let's prepend/append something to items for submenus. We can't distinguish top-level menu items and sub-menu items in resultant strings by this code. For example, we'd like to have a string vector like: "Item 1" "Item 2" "Item 3" "Submenu >" "_Item 4" "_Deeper submenu >" "__Item 5" "__Item 6" "Item 7" In this example, we prepend "_"s to items and it represents menu depth, and append " >" to submenu items.
Added submenu and menu depth identifier as suggested. https://codereview.chromium.org/438193003/diff/1/content/shell/renderer/test_... File content/shell/renderer/test_runner/event_sender.cc (right): https://codereview.chromium.org/438193003/diff/1/content/shell/renderer/test_... content/shell/renderer/test_runner/event_sender.cc:136: PopulateCustomItems(customItems[i].subMenuItems, strings); On 2014/08/05 06:36:57, tkent wrote: > Let's prepend/append something to items for submenus. > We can't distinguish top-level menu items and sub-menu items in resultant > strings by this code. > > For example, we'd like to have a string vector like: > "Item 1" > "Item 2" > "Item 3" > "Submenu >" > "_Item 4" > "_Deeper submenu >" > "__Item 5" > "__Item 6" > "Item 7" > > In this example, we prepend "_"s to items and it represents menu depth, and > append " >" to submenu items. Done.
https://codereview.chromium.org/438193003/diff/20001/content/shell/renderer/t... File content/shell/renderer/test_runner/event_sender.cc (right): https://codereview.chromium.org/438193003/diff/20001/content/shell/renderer/t... content/shell/renderer/test_runner/event_sender.cc:125: const std::string kSubMenuIdentifier = " >"; We don't allow static initialization of classes. kSubMenuIdentifier and kSubMenuDepthIdentifier should be moved into functions, or their type should be const char[]. https://codereview.chromium.org/438193003/diff/20001/content/shell/renderer/t... content/shell/renderer/test_runner/event_sender.cc:146: std::vector<std::string>& strings, unsigned depth) { Please make the third argument |const std::string& prefix| and apply following comments. They will make the code simpler. https://codereview.chromium.org/438193003/diff/20001/content/shell/renderer/t... content/shell/renderer/test_runner/event_sender.cc:150: strings.push_back(depthPrefix(newDepth) + customItems[i].label.utf8() + kSubMenuIdentifier); |depthPrefix(newDepth) +| -> |prefix +| https://codereview.chromium.org/438193003/diff/20001/content/shell/renderer/t... content/shell/renderer/test_runner/event_sender.cc:151: PopulateCustomItems(customItems[i].subMenuItems, strings, newDepth); |newDepth| -> |prefix + kSubMenuDepthIdentifier| https://codereview.chromium.org/438193003/diff/20001/content/shell/renderer/t... content/shell/renderer/test_runner/event_sender.cc:153: strings.push_back(depthPrefix(depth) + customItems[i].label.utf8()); |depthPrefix(newDepth) +| -> |prefix +| https://codereview.chromium.org/438193003/diff/20001/content/shell/renderer/t... content/shell/renderer/test_runner/event_sender.cc:203: // Populate custom menu items if provided by the WebCore internals. |WebCore| is an obsolete name. "provided by Blink." https://codereview.chromium.org/438193003/diff/20001/content/shell/renderer/t... content/shell/renderer/test_runner/event_sender.cc:204: PopulateCustomItems(context_menu->customItems, strings, 0); |0| -> |""|
Updated the patch. PTAL. https://codereview.chromium.org/438193003/diff/20001/content/shell/renderer/t... File content/shell/renderer/test_runner/event_sender.cc (right): https://codereview.chromium.org/438193003/diff/20001/content/shell/renderer/t... content/shell/renderer/test_runner/event_sender.cc:125: const std::string kSubMenuIdentifier = " >"; On 2014/08/05 08:50:10, tkent wrote: > We don't allow static initialization of classes. > kSubMenuIdentifier and kSubMenuDepthIdentifier should be moved into functions, > or their type should be const char[]. Done. https://codereview.chromium.org/438193003/diff/20001/content/shell/renderer/t... content/shell/renderer/test_runner/event_sender.cc:146: std::vector<std::string>& strings, unsigned depth) { On 2014/08/05 08:50:10, tkent wrote: > Please make the third argument |const std::string& prefix| and apply following > comments. > > They will make the code simpler. Done. Thanks https://codereview.chromium.org/438193003/diff/20001/content/shell/renderer/t... content/shell/renderer/test_runner/event_sender.cc:150: strings.push_back(depthPrefix(newDepth) + customItems[i].label.utf8() + kSubMenuIdentifier); On 2014/08/05 08:50:10, tkent wrote: > |depthPrefix(newDepth) +| -> |prefix +| > Done. https://codereview.chromium.org/438193003/diff/20001/content/shell/renderer/t... content/shell/renderer/test_runner/event_sender.cc:151: PopulateCustomItems(customItems[i].subMenuItems, strings, newDepth); On 2014/08/05 08:50:10, tkent wrote: > |newDepth| -> |prefix + kSubMenuDepthIdentifier| Done. https://codereview.chromium.org/438193003/diff/20001/content/shell/renderer/t... content/shell/renderer/test_runner/event_sender.cc:153: strings.push_back(depthPrefix(depth) + customItems[i].label.utf8()); On 2014/08/05 08:50:11, tkent wrote: > |depthPrefix(newDepth) +| -> |prefix +| Done. https://codereview.chromium.org/438193003/diff/20001/content/shell/renderer/t... content/shell/renderer/test_runner/event_sender.cc:203: // Populate custom menu items if provided by the WebCore internals. On 2014/08/05 08:50:11, tkent wrote: > |WebCore| is an obsolete name. > > "provided by Blink." Done. https://codereview.chromium.org/438193003/diff/20001/content/shell/renderer/t... content/shell/renderer/test_runner/event_sender.cc:204: PopulateCustomItems(context_menu->customItems, strings, 0); On 2014/08/05 08:50:10, tkent wrote: > |0| -> |""| Done.
https://codereview.chromium.org/438193003/diff/40001/content/shell/renderer/t... File content/shell/renderer/test_runner/event_sender.cc (right): https://codereview.chromium.org/438193003/diff/40001/content/shell/renderer/t... content/shell/renderer/test_runner/event_sender.cc:133: void PopulateCustomItems(const WebVector<WebMenuItemInfo>& customItems, This doesn't follow Chromium coding style. - 'out' argument must be a pointer, not a reference - 'out' argument must be the last argument. So, this function should be: PopulateCustomItems(const WebVector<WebMenuItemInfo>& customItems, const std::string& prefix, std::vector<std::string>* strings) https://codereview.chromium.org/438193003/diff/40001/content/shell/renderer/t... content/shell/renderer/test_runner/event_sender.cc:137: strings.push_back(prefix + customItems[i].label.utf8() + kSubMenuIdentifier); wrap in 80 columns https://codereview.chromium.org/438193003/diff/40001/content/shell/renderer/t... content/shell/renderer/test_runner/event_sender.cc:138: PopulateCustomItems(customItems[i].subMenuItems, strings, prefix + kSubMenuDepthIdentifier); Ditto. https://codereview.chromium.org/438193003/diff/40001/content/shell/renderer/t... content/shell/renderer/test_runner/event_sender.cc:190: // Populate custom menu items if provided by the blink internals. nit: |by the blink internals.| -> |by Blink.|
Please take a look. https://codereview.chromium.org/438193003/diff/40001/content/shell/renderer/t... File content/shell/renderer/test_runner/event_sender.cc (right): https://codereview.chromium.org/438193003/diff/40001/content/shell/renderer/t... content/shell/renderer/test_runner/event_sender.cc:133: void PopulateCustomItems(const WebVector<WebMenuItemInfo>& customItems, On 2014/08/05 09:08:45, tkent wrote: > This doesn't follow Chromium coding style. > - 'out' argument must be a pointer, not a reference > - 'out' argument must be the last argument. > > So, this function should be: > PopulateCustomItems(const WebVector<WebMenuItemInfo>& customItems, const > std::string& prefix, std::vector<std::string>* strings) Done. https://codereview.chromium.org/438193003/diff/40001/content/shell/renderer/t... content/shell/renderer/test_runner/event_sender.cc:137: strings.push_back(prefix + customItems[i].label.utf8() + kSubMenuIdentifier); On 2014/08/05 09:08:45, tkent wrote: > wrap in 80 columns Done. https://codereview.chromium.org/438193003/diff/40001/content/shell/renderer/t... content/shell/renderer/test_runner/event_sender.cc:138: PopulateCustomItems(customItems[i].subMenuItems, strings, prefix + kSubMenuDepthIdentifier); On 2014/08/05 09:08:45, tkent wrote: > Ditto. Done. https://codereview.chromium.org/438193003/diff/40001/content/shell/renderer/t... content/shell/renderer/test_runner/event_sender.cc:190: // Populate custom menu items if provided by the blink internals. On 2014/08/05 09:08:45, tkent wrote: > nit: |by the blink internals.| -> |by Blink.| Done.
lgtm https://codereview.chromium.org/438193003/diff/60001/content/shell/renderer/t... File content/shell/renderer/test_runner/event_sender.cc (right): https://codereview.chromium.org/438193003/diff/60001/content/shell/renderer/t... content/shell/renderer/test_runner/event_sender.cc:192: // Populate custom menu items if provided by the blink. nit: |by the blink| -> |by Blink|
https://codereview.chromium.org/438193003/diff/60001/content/shell/renderer/t... File content/shell/renderer/test_runner/event_sender.cc (right): https://codereview.chromium.org/438193003/diff/60001/content/shell/renderer/t... content/shell/renderer/test_runner/event_sender.cc:192: // Populate custom menu items if provided by the blink. On 2014/08/05 09:18:39, tkent wrote: > nit: |by the blink| -> |by Blink| Done.
The CQ bit was checked by tkent@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sanjoy.pal@samsung.com/438193003/80001
Message was sent while issue was closed.
Change committed as 287527 |
