Adding (empty) placeholder button to 'Other bookmarks' folder is unnecessary.

Adding (empty) placeholder button to 'Other bookmarks' folder is unnecessary.

When user removes the last item (folder/page) from 'Other bookmarks' folder,
the folder itself is hidden; hence adding (empty) button is unnecessary. So
just close the folder before hiding it.

TEST=
1) Launch chrome and bookmark a page or folder into 'Other Bookmarks'.
2) Right-click on the bookmarked page/folder in the 'Other Bookmarks' from Bookmark bar.
3) Choose to 'Cut'/'Delete' and observe.
4) (empty) placeholder button should not remain after 'Other bookmarks' folder is hidden.

BUG=257538
R=asvitkine@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=291160

[Gaja, 2014-08-19 07:55:59]

PTAL.

[Alexei Svitkine (slow), 2014-08-19 13:45:10]

Can you reword your CL description to more closely match the project convention?

First line should summarize the change (not the bug) in a single line (under 80
chars). for e.g. "[Mac] Add (empty) item after last item is deleted from
bookmark folder.".

Then more details can be put in the rest of the description.

Finally, if it's a user visible change (which this is), include a TEST= block
with instructions that QA can use to verify the fix (i.e. repro steps and
expected behaviour).

[Gaja, 2014-08-19 16:15:39]

On 2014/08/19 13:45:10, Alexei Svitkine wrote:
> Can you reword your CL description to more closely match the project
convention?
> 
> First line should summarize the change (not the bug) in a single line (under
80
> chars). for e.g. "[Mac] Add (empty) item after last item is deleted from
> bookmark folder.".
> 
> Then more details can be put in the rest of the description.
> 
> Finally, if it's a user visible change (which this is), include a TEST= block
> with instructions that QA can use to verify the fix (i.e. repro steps and
> expected behaviour).

Thanks for suggesting. I have updated CL description addressing your comments.
PTAL.

[Alexei Svitkine (slow), 2014-08-19 17:01:38]

https://codereview.chromium.org/485003002/diff/1/chrome/browser/ui/cocoa/book...
File chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_controller.mm
(right):

https://codereview.chromium.org/485003002/diff/1/chrome/browser/ui/cocoa/book...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_controller.mm:763: if
(buttonCount == 0) {
Please add a comment explaining this logic.

https://codereview.chromium.org/485003002/diff/1/chrome/browser/ui/cocoa/book...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_controller.mm:764: [self
adjustWindowLeft:0 size:NSMakeSize(0, 0) scrollingBy:0.0];
Why make the window 0 size instead of calling -closeBookmarkFolder:?

Also, I think 0-size windows cause problems on the Mac.

[Gaja, 2014-08-20 05:51:32]

Changes in patch set 2, as per the comments. Please review.

https://codereview.chromium.org/485003002/diff/1/chrome/browser/ui/cocoa/book...
File chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_controller.mm
(right):

https://codereview.chromium.org/485003002/diff/1/chrome/browser/ui/cocoa/book...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_controller.mm:763: if
(buttonCount == 0) {
On 2014/08/19 17:01:38, Alexei Svitkine wrote:
> Please add a comment explaining this logic.

Moved this part of code into -removeButton function.

https://codereview.chromium.org/485003002/diff/1/chrome/browser/ui/cocoa/book...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_controller.mm:764: [self
adjustWindowLeft:0 size:NSMakeSize(0, 0) scrollingBy:0.0];
On 2014/08/19 17:01:38, Alexei Svitkine wrote:
> Why make the window 0 size instead of calling -closeBookmarkFolder:?
> 
> Also, I think 0-size windows cause problems on the Mac.

I do think so. Changes in patch set 2, please review. Thanks.

[Alexei Svitkine (slow), 2014-08-20 14:46:53]

Change looks good now. Is it possible to add a test for this?

[Gaja, 2014-08-21 04:41:31]

On 2014/08/20 14:46:53, Alexei Svitkine wrote:
> Change looks good now. Is it possible to add a test for this?

Sure, I will upload the same in patch set 3.

[Gaja, 2014-08-21 11:33:34]

On 2014/08/20 14:46:53, Alexei Svitkine wrote:
> Change looks good now. Is it possible to add a test for this?

Added unit test for the patch. Please review.

[Alexei Svitkine (slow), 2014-08-21 13:41:42]

https://codereview.chromium.org/485003002/diff/40001/chrome/browser/ui/cocoa/...
File
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_controller_unittest.mm
(right):

https://codereview.chromium.org/485003002/diff/40001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_controller_unittest.mm:1177:
[[otherButton target] performSelector:@selector(openBookmarkFolderFromButton:)
Nit: Why performSelector: and not just call the method directly?

[Gaja, 2014-08-21 14:45:43]

https://codereview.chromium.org/485003002/diff/40001/chrome/browser/ui/cocoa/...
File
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_controller_unittest.mm
(right):

https://codereview.chromium.org/485003002/diff/40001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_controller_unittest.mm:1177:
[[otherButton target] performSelector:@selector(openBookmarkFolderFromButton:)
On 2014/08/21 13:41:42, Alexei Svitkine wrote:
> Nit: Why performSelector: and not just call the method directly?

In almost all test cases performSelector: has been used when calling
openBookmarkFolderFromButton:. I had built and ran the test case without using
it and worked fine(Test passed). Do you want me to remove it?

[Alexei Svitkine (slow), 2014-08-21 14:48:03]

LGTM % addressing that comment. Thanks!

https://codereview.chromium.org/485003002/diff/40001/chrome/browser/ui/cocoa/...
File
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_controller_unittest.mm
(right):

https://codereview.chromium.org/485003002/diff/40001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_controller_unittest.mm:1177:
[[otherButton target] performSelector:@selector(openBookmarkFolderFromButton:)
On 2014/08/21 14:45:43, Gajendra N wrote:
> On 2014/08/21 13:41:42, Alexei Svitkine wrote:
> > Nit: Why performSelector: and not just call the method directly?
> 
> In almost all test cases performSelector: has been used when calling
> openBookmarkFolderFromButton:. I had built and ran the test case without using
> it and worked fine(Test passed). Do you want me to remove it?

Yes, please. It's unnecessary.

[Gaja, 2014-08-21 14:49:11]

On 2014/08/21 14:48:03, Alexei Svitkine wrote:
> LGTM % addressing that comment. Thanks!
> 
>
https://codereview.chromium.org/485003002/diff/40001/chrome/browser/ui/cocoa/...
> File
> chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_controller_unittest.mm
> (right):
> 
>
https://codereview.chromium.org/485003002/diff/40001/chrome/browser/ui/cocoa/...
>
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_controller_unittest.mm:1177:
> [[otherButton target] performSelector:@selector(openBookmarkFolderFromButton:)
> On 2014/08/21 14:45:43, Gajendra N wrote:
> > On 2014/08/21 13:41:42, Alexei Svitkine wrote:
> > > Nit: Why performSelector: and not just call the method directly?
> > 
> > In almost all test cases performSelector: has been used when calling
> > openBookmarkFolderFromButton:. I had built and ran the test case without
using
> > it and worked fine(Test passed). Do you want me to remove it?
> 
> Yes, please. It's unnecessary.

Okay. Thanks!

[Gaja, 2014-08-21 14:53:54]

The CQ bit was checked by gajendra.n@samsung.com

[commit-bot: I haz the power, 2014-08-21 14:55:08]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/gajendra.n@samsung.com/485003002/60001

[Gaja, 2014-08-21 15:01:24]

https://codereview.chromium.org/485003002/diff/40001/chrome/browser/ui/cocoa/...
File
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_controller_unittest.mm
(right):

https://codereview.chromium.org/485003002/diff/40001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_controller_unittest.mm:1177:
[[otherButton target] performSelector:@selector(openBookmarkFolderFromButton:)
On 2014/08/21 14:48:03, Alexei Svitkine wrote:
> On 2014/08/21 14:45:43, Gajendra N wrote:
> > On 2014/08/21 13:41:42, Alexei Svitkine wrote:
> > > Nit: Why performSelector: and not just call the method directly?
> > 
> > In almost all test cases performSelector: has been used when calling
> > openBookmarkFolderFromButton:. I had built and ran the test case without
using
> > it and worked fine(Test passed). Do you want me to remove it?
> 
> Yes, please. It's unnecessary. 

Done.

[commit-bot: I haz the power, 2014-08-21 19:04:49]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[commit-bot: I haz the power, 2014-08-21 20:16:11]

Committed patchset #4 (60001) as 291160