Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2713333002:
Remove redundant separator at end of the spelling correction context menu (Closed)
|
Created:
3 years, 10 months ago by edwardjung Modified:
3 years, 9 months ago Reviewers:
lazyboy CC:
chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove redundant separator at end of the spelling correction context menu
In some editing cases (reporting a issue) inspect is not the last item.
BUG=695800
Review-Url: https://codereview.chromium.org/2713333002
Cr-Commit-Position: refs/heads/master@{#453565}
Committed: https://chromium.googlesource.com/chromium/src/+/0f26be86266baa146ff07b69aace687393d8fc36
Patch Set 1 #Patch Set 2 : Remove redundant separator at end of context menu #Patch Set 3 : Just remove the separator #
Total comments: 2
Patch Set 4 : Switch back to check for separators at end of menu init #
Total comments: 2
Patch Set 5 : Fix nit #Messages
Total messages: 24 (16 generated)
The CQ bit was checked by edwardjung@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Remove redundant separator at end of context menu BUG=695800 ========== to ========== Remove redundant separator at end of the spelling correction context menu In some editing cases (reporting a issue) inspect is not the last item. BUG=695800 ==========
The CQ bit was checked by edwardjung@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
edwardjung@chromium.org changed reviewers: + lazyboy@chromium.org
lazyboy PTAL. Thanks.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2713333002/diff/40001/chrome/browser/renderer... File chrome/browser/renderer_context_menu/render_view_context_menu.cc (left): https://codereview.chromium.org/2713333002/diff/40001/chrome/browser/renderer... chrome/browser/renderer_context_menu/render_view_context_menu.cc:1414: menu_model_.AddSeparator(ui::NORMAL_SEPARATOR); This is prone to further bugs I think: If group items that come after AppendEditableItems() do not prepend a separator then we will have two groups unintentionally merged? Instead we can remove a separator at the end of ::InitMenu(), sth like: if (menu_model_.GetItemCount() > 0 && menu_model_.GetTypeAt(menu_model_.GetItemCount()-1) == TYPE_SEPARATOR) menu_model_.RemoveItemAt(...) WDYT?
On 2017/02/28 00:29:02, lazyboy wrote: > https://codereview.chromium.org/2713333002/diff/40001/chrome/browser/renderer... > File chrome/browser/renderer_context_menu/render_view_context_menu.cc (left): > > https://codereview.chromium.org/2713333002/diff/40001/chrome/browser/renderer... > chrome/browser/renderer_context_menu/render_view_context_menu.cc:1414: > menu_model_.AddSeparator(ui::NORMAL_SEPARATOR); > This is prone to further bugs I think: > If group items that come after AppendEditableItems() do not prepend a separator > then we will have two groups unintentionally merged? > > Instead we can remove a separator at the end of ::InitMenu(), sth like: > if (menu_model_.GetItemCount() > 0 && > menu_model_.GetTypeAt(menu_model_.GetItemCount()-1) == TYPE_SEPARATOR) > menu_model_.RemoveItemAt(...) > > WDYT? Funnily enough this is how I had it in Patchset #2 https://codereview.chromium.org/2713333002/diff/20001/chrome/browser/renderer... But then changed it thinking this is a simpler option. You are right though as any new sections would need to add a separator. I'll follow up now with an new patchset,
Updated per suggestion. https://codereview.chromium.org/2713333002/diff/40001/chrome/browser/renderer... File chrome/browser/renderer_context_menu/render_view_context_menu.cc (left): https://codereview.chromium.org/2713333002/diff/40001/chrome/browser/renderer... chrome/browser/renderer_context_menu/render_view_context_menu.cc:1414: menu_model_.AddSeparator(ui::NORMAL_SEPARATOR); On 2017/02/28 00:29:02, lazyboy wrote: > This is prone to further bugs I think: > If group items that come after AppendEditableItems() do not prepend a separator > then we will have two groups unintentionally merged? > > Instead we can remove a separator at the end of ::InitMenu(), sth like: > if (menu_model_.GetItemCount() > 0 && > menu_model_.GetTypeAt(menu_model_.GetItemCount()-1) == TYPE_SEPARATOR) > menu_model_.RemoveItemAt(...) > > WDYT? Done.
lgtm https://codereview.chromium.org/2713333002/diff/60001/chrome/browser/renderer... File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right): https://codereview.chromium.org/2713333002/diff/60001/chrome/browser/renderer... chrome/browser/renderer_context_menu/render_view_context_menu.cc:857: // Remove any redundant spearators. nit: trailing separator.
The CQ bit was checked by edwardjung@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2713333002/diff/60001/chrome/browser/renderer... File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right): https://codereview.chromium.org/2713333002/diff/60001/chrome/browser/renderer... chrome/browser/renderer_context_menu/render_view_context_menu.cc:857: // Remove any redundant spearators. On 2017/02/28 01:03:29, lazyboy wrote: > nit: trailing separator. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by edwardjung@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lazyboy@chromium.org Link to the patchset: https://codereview.chromium.org/2713333002/#ps80001 (title: "Fix nit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1488277545065440,
"parent_rev": "a08766e7f8ec19cc09341d18b91107377ccfbcd6", "commit_rev":
"0f26be86266baa146ff07b69aace687393d8fc36"}
Message was sent while issue was closed.
Description was changed from ========== Remove redundant separator at end of the spelling correction context menu In some editing cases (reporting a issue) inspect is not the last item. BUG=695800 ========== to ========== Remove redundant separator at end of the spelling correction context menu In some editing cases (reporting a issue) inspect is not the last item. BUG=695800 Review-Url: https://codereview.chromium.org/2713333002 Cr-Commit-Position: refs/heads/master@{#453565} Committed: https://chromium.googlesource.com/chromium/src/+/0f26be86266baa146ff07b69aace... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/0f26be86266baa146ff07b69aace... |
