Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
606593002:
Move bookmark's folder menu upwards only when its parent button is moved (Closed)
|
Created:
6 years, 2 months ago by Gaja Modified:
6 years, 2 months ago Reviewers:
Alexei Svitkine (slow) CC:
chromium-reviews, tfarina Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionMove bookmark's folder menu upwards only when its parent button is moved.
On dragging and dropping an item on a folder that is
currently showing its folder menu; the menu moves upwards
by an offset, regardless of its parent button moved or not.
This CL fixes the issue that the folder menu is moved only
when its parent button is moved. Also, the offset value
should be kBookmarkFolderButtonHeight.
BUG=311155
R=asvitkine@chromium.org
TEST=
1) Launch chrome and add a bookmark folder(folder1) to Bookmarks Bar
2) Add two folders(folder2 and folder3) within folder1.
3) Open folder1, now drag folder3 and hover it on folder2,
wait until folder2 opens its folder menu, now drop the
folder i.e release the mouse, and observe.
4) folder2's folder menu should not shift upwards, it
should horizontally align with its button's frame.
Committed: https://crrev.com/333c2298ec9bf673294fe75f6a819c032bb2155c
Cr-Commit-Position: refs/heads/master@{#299037}
Patch Set 1 #
Total comments: 11
Patch Set 2 : Addressing comments and refactor code covering one more test case. #
Total comments: 4
Patch Set 3 : Reverting to previous loop logic but adding index check. #Patch Set 4 : nit : additional space #
Total comments: 4
Patch Set 5 : Addressing comments #
Total comments: 1
Messages
Total messages: 14 (1 generated)
@asvitkine, Please take a look. Thanks. https://codereview.chromium.org/606593002/diff/1/chrome/browser/ui/cocoa/book... File chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_controller.mm (left): https://codereview.chromium.org/606593002/diff/1/chrome/browser/ui/cocoa/book... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_controller.mm:1942: offsetFolderMenuWindow:NSMakeSize(0.0, chrome::kBookmarkBarHeight)]; @asvitkine, This CL is to remove only the above 'if' block. Along with that I have re-factored the below 'if else' code block to improve code readability. PTAL.
https://codereview.chromium.org/606593002/diff/1/chrome/browser/ui/cocoa/book... File chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_controller.mm (left): https://codereview.chromium.org/606593002/diff/1/chrome/browser/ui/cocoa/book... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_controller.mm:1939: // If this button is showing its menu then we need to move the menu, too. Are you sure we never need to do this? What if you're moving folder2 to folder3 instead, such that folder3 is below and thus needs to shift upward? Is it possible to add some tests for all these cases? https://codereview.chromium.org/606593002/diff/1/chrome/browser/ui/cocoa/book... File chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_controller.mm (right): https://codereview.chromium.org/606593002/diff/1/chrome/browser/ui/cocoa/book... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_controller.mm:1935: frame:buttonFrame]; Nit: Align :'s or wrap after =. https://codereview.chromium.org/606593002/diff/1/chrome/browser/ui/cocoa/book... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_controller.mm:1938: buttonCount = 1; Shouldn't this block call [self adjustWindowForButtonCount:buttonCount]; too like the previous code? Or the "if buttonCount > 0" block shouldn't be an else if below. https://codereview.chromium.org/606593002/diff/1/chrome/browser/ui/cocoa/book... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_controller.mm:1940: else { Nit: Put on previous line. https://codereview.chromium.org/606593002/diff/1/chrome/browser/ui/cocoa/book... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_controller.mm:1945: } else if (buttonCount > 0) Nit: {}'s
https://codereview.chromium.org/606593002/diff/1/chrome/browser/ui/cocoa/book... File chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_controller.mm (left): https://codereview.chromium.org/606593002/diff/1/chrome/browser/ui/cocoa/book... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_controller.mm:1939: // If this button is showing its menu then we need to move the menu, too. On 2014/09/26 15:14:37, Alexei Svitkine wrote: > Are you sure we never need to do this? > > What if you're moving folder2 to folder3 instead, such that folder3 is below and > thus needs to shift upward? > > Is it possible to add some tests for all these cases? Oh, I did not test this case, I will check the behavior and update the same(on monday :( ). Also I will add test cases if necessary. Thanks! https://codereview.chromium.org/606593002/diff/1/chrome/browser/ui/cocoa/book... File chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_controller.mm (right): https://codereview.chromium.org/606593002/diff/1/chrome/browser/ui/cocoa/book... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_controller.mm:1935: frame:buttonFrame]; On 2014/09/26 15:14:37, Alexei Svitkine wrote: > Nit: Align :'s or wrap after =. Acknowledged. https://codereview.chromium.org/606593002/diff/1/chrome/browser/ui/cocoa/book... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_controller.mm:1938: buttonCount = 1; On 2014/09/26 15:14:37, Alexei Svitkine wrote: > Shouldn't this block call [self adjustWindowForButtonCount:buttonCount]; too > like the previous code? > > Or the "if buttonCount > 0" block shouldn't be an else if below. I think we don't need this line. Here is my analysis, FMPOV, if we are at this point, it means that we just removed last button and adding (empty) placeholder button, so we don't need to call [self adjustWindowForButtonCount:buttonCount];, that is we don't need to adjust the window because we removed one button and added one button at the same place(height). Since "[self adjustWindowForButtonCount:buttonCount];" is in 'else if' block, it is never called when we are removing last button. In case of "Other bookmarks" button (else {} part), when we remove last button, we do not add '(empty)' placeholder button, but we close the folder and hide it, therefore, again we don't need to call "[self adjustWindowForButtonCount:buttonCount];". Calling [self adjustWindowForButtonCount:buttonCount]; is necessary only when the removed button is not the last button in that folder. Please correct me if I was wrong anywhere :). https://codereview.chromium.org/606593002/diff/1/chrome/browser/ui/cocoa/book... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_controller.mm:1940: else { On 2014/09/26 15:14:37, Alexei Svitkine wrote: > Nit: Put on previous line. Acknowledged. https://codereview.chromium.org/606593002/diff/1/chrome/browser/ui/cocoa/book... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_controller.mm:1945: } else if (buttonCount > 0) On 2014/09/26 15:14:37, Alexei Svitkine wrote: > Nit: {}'s Acknowledged.
@asvitkine, Please take a look at Patch Set #2. Writing test for this CL seems bit difficult, as we have to wait for the target folder to get opened between dragginEntered and draggingExited events. Also, there are no test cases currently available related to these operations/events. (It is under // TODO).
https://codereview.chromium.org/606593002/diff/20001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_controller.mm (right): https://codereview.chromium.org/606593002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_controller.mm:1937: // In case of Drag & Drop within a folder, if the target folder is showing I'm not sure this behavior (as described by the comment and what you implemented) is sufficient. This is the general remove method and may potentially be used in other circumstances than drag and drop. So a different folder menu could be open. For example, an entry could get removed while you have the menu open as a result of sync from another machine. So I think it's safer to have the previous logic that loops through the buttons, but to do a check against the index as you do here. https://codereview.chromium.org/606593002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_controller.mm:1944: if ((dropIndex + 1) > buttonIndex) Nit: {}'s
@asvitkine Comments addressed in Patch set #3. Please review. https://codereview.chromium.org/606593002/diff/20001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_controller.mm (right): https://codereview.chromium.org/606593002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_controller.mm:1937: // In case of Drag & Drop within a folder, if the target folder is showing On 2014/10/02 20:55:59, Alexei Svitkine wrote: > I'm not sure this behavior (as described by the comment and what you > implemented) is sufficient. > > This is the general remove method and may potentially be used in other > circumstances than drag and drop. So a different folder menu could be open. > > For example, an entry could get removed while you have the menu open as a result > of sync from another machine. > > So I think it's safer to have the previous logic that loops through the buttons, > but to do a check against the index as you do here. Done. https://codereview.chromium.org/606593002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_controller.mm:1944: if ((dropIndex + 1) > buttonIndex) On 2014/10/02 20:55:59, Alexei Svitkine wrote: > Nit: {}'s Done.
https://codereview.chromium.org/606593002/diff/60001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_controller.mm (right): https://codereview.chromium.org/606593002/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_controller.mm:1941: NSInteger targetIndex = [buttons_ indexOfObject:aButton]; This results in O(n^2) behavior, since -indexOfObject: will find the index from the array again. Can you avoid this? Either add an index variable and increment it in the loop or alternatively change it to a for (int i = 0; ... loop. https://codereview.chromium.org/606593002/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_controller.mm:1942: if ((targetIndex + 1) > buttonIndex) { Can you add a comment explaining this? Maybe if you combine with the above if statement once index is being tracked, just add to the existing comment above.
Changes in Patch Set #5. Please review. Thanks. https://codereview.chromium.org/606593002/diff/60001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_controller.mm (right): https://codereview.chromium.org/606593002/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_controller.mm:1941: NSInteger targetIndex = [buttons_ indexOfObject:aButton]; On 2014/10/06 15:37:30, Alexei Svitkine wrote: > This results in O(n^2) behavior, since -indexOfObject: will find the index from > the array again. > > Can you avoid this? Either add an index variable and increment it in the loop or > alternatively change it to a for (int i = 0; ... loop. Done. Making use of an index variable by incrementing it in loop. https://codereview.chromium.org/606593002/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_controller.mm:1942: if ((targetIndex + 1) > buttonIndex) { On 2014/10/06 15:37:30, Alexei Svitkine wrote: > Can you add a comment explaining this? > > Maybe if you combine with the above if statement once index is being tracked, > just add to the existing comment above. Done. I have added it to existing comment above. https://codereview.chromium.org/606593002/diff/80001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_controller.mm (right): https://codereview.chromium.org/606593002/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_controller.mm:1946: break; Is it okay to break the loop once we found the button and moved its menu? I hope it does not cause any other problems.
On 2014/10/07 04:39:12, gaja wrote: > Changes in Patch Set #5. Please review. Thanks. > > https://codereview.chromium.org/606593002/diff/60001/chrome/browser/ui/cocoa/... > File chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_controller.mm > (right): > > https://codereview.chromium.org/606593002/diff/60001/chrome/browser/ui/cocoa/... > chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_controller.mm:1941: > NSInteger targetIndex = [buttons_ indexOfObject:aButton]; > On 2014/10/06 15:37:30, Alexei Svitkine wrote: > > This results in O(n^2) behavior, since -indexOfObject: will find the index > from > > the array again. > > > > Can you avoid this? Either add an index variable and increment it in the loop > or > > alternatively change it to a for (int i = 0; ... loop. > > Done. Making use of an index variable by incrementing it in loop. > > https://codereview.chromium.org/606593002/diff/60001/chrome/browser/ui/cocoa/... > chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_controller.mm:1942: if > ((targetIndex + 1) > buttonIndex) { > On 2014/10/06 15:37:30, Alexei Svitkine wrote: > > Can you add a comment explaining this? > > > > Maybe if you combine with the above if statement once index is being tracked, > > just add to the existing comment above. > > Done. I have added it to existing comment above. > > https://codereview.chromium.org/606593002/diff/80001/chrome/browser/ui/cocoa/... > File chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_controller.mm > (right): > > https://codereview.chromium.org/606593002/diff/80001/chrome/browser/ui/cocoa/... > chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_controller.mm:1946: break; > Is it okay to break the loop once we found the button and moved its menu? > I hope it does not cause any other problems. PTAL.
lgtm
The CQ bit was checked by gajendra.n@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/606593002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/333c2298ec9bf673294fe75f6a819c032bb2155c Cr-Commit-Position: refs/heads/master@{#299037} |
