Fix drag and drop bookmark reordering [chromium/src : main]
Yuheng Huang (Gerrit)
Yuheng Huang added 1 comment![]( "Open in Gerrit")
menu->AppendSeparator();I wonder if we can mark this element as LAST_PERMANENT_MENU_ITEM_ID with View::SetID() and get the index from the menu to add to from AddBookmarkNode instead of hard coding the offset?
Related details
- Dominic Austria
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yuheng Huang (Gerrit)
Yuheng Huang added 4 comments![]( "Open in Gerrit")
bool ShouldAppendOpenAllCommands(const BookmarkParentFolder& folder) {This function is a bit misleading. Even if it returns true, if the bookmark folder has no bookmarks we don't append open all commands. Same as AppendOpenAllCommandItems(). Can just inline all the code like before since it does not need to reuse it elsewhere.
// TODO use ElementTrackerviews perhaps here
if (!menu->HasSubmenu()) {
return 0;
}What case is this? Can we remove it if the code below already handle it?
if (ShouldAppendOpenAllCommands(new_parent_folder)) {I think we don't need to check here. If the separator is not found we can just don't add the offset.
void BookmarkMenuDelegate::AppendOpenAllCommandItems(Also inline this?
Related details
- Dominic Austria
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dominic Austria (Gerrit)
Dominic Austria added 4 comments![]( "Open in Gerrit")
bool ShouldAppendOpenAllCommands(const BookmarkParentFolder& folder) {This function is a bit misleading. Even if it returns true, if the bookmark folder has no bookmarks we don't append open all commands. Same as AppendOpenAllCommandItems(). Can just inline all the code like before since it does not need to reuse it elsewhere.
Done
// TODO use ElementTrackerviews perhaps here
if (!menu->HasSubmenu()) {
return 0;
}What case is this? Can we remove it if the code below already handle it?
I am just worried that if it doesn't have a Submenu, we will crash on the next line where we dereference menu->GetSubmenu(). Not sure when this will happen though.
if (ShouldAppendOpenAllCommands(new_parent_folder)) {I think we don't need to check here. If the separator is not found we can just don't add the offset.
Done
I wonder if we can mark this element as LAST_PERMANENT_MENU_ITEM_ID with View::SetID() and get the index from the menu to add to from AddBookmarkNode instead of hard coding the offset?
- Yuheng Huang
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dominic Austria (Gerrit)
Dominic Austria added 1 comment![]( "Open in Gerrit")
void BookmarkMenuDelegate::AppendOpenAllCommandItems(Also inline this?
I guess I could, I know this function isnt used anywhwere else but I wanted to keep BookmarkMenuDelegate::BuildMenu small and readable
Related details
- David Pennington
- Yuheng Huang
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dominic Austria (Gerrit)
Dominic Austria added 2 comments![]( "Open in Gerrit")
if (!menu->HasSubmenu()) {I probably can remove this but I am just worried that if it doesn't have a Submenu, we will crash on the next line where we dereference menu->GetSubmenu(). Not sure when this will happen though.
// TODO use ElementTrackerviews perhaps here
if (!menu->HasSubmenu()) {
return 0;
}Dominic AustriaWhat case is this? Can we remove it if the code below already handle it?
I am just worried that if it doesn't have a Submenu, we will crash on the next line where we dereference menu->GetSubmenu(). Not sure when this will happen though.
made a new thread since i updated the code section
Related details
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Pennington (Gerrit)
David Pennington added 1 comment![]( "Open in Gerrit")
menu_items[1]->SetEnabled(false);do we want to update the strings as well in this case? if theres a way to get rid of the if else expression i would prefer that since its less error prone.
Related details
- Dominic Austria
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dominic Austria (Gerrit)
Dominic Austria added 1 comment![]( "Open in Gerrit")
menu_items[1]->SetEnabled(false);do we want to update the strings as well in this case? if theres a way to get rid of the if else expression i would prefer that since its less error prone.
It turns out when I call GetPluralStringFUTF16(IDS_BOOKMARK_BAR_OPEN_ALL_COUNT, open_count) with open_count = 0 , the result is 'Open All'. So when updating the string, it transitions from 'Open Bookmark' (in enabled state) to 'Open All' in a disabled state. I wrote this if else condition because I thought it was nicer to disable the menu item without the string text updating. In case you don't know what I'm talking about, the video I sent you shows what I'm trying to describe more clearly
Related details
- David Pennington
- Yuheng Huang
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |