Bookmarks bar, tooltip should be seen only for folders with big names on macOS.

Bookmarks bar, unnecessary tooltip is seen for folders with small name.

BUG=635836
Committed: https://crrev.com/efa1d76a70dfec345ff89d2ca978a46029c51e7e
Cr-Commit-Position: refs/heads/master@{#433563}

[dhna, 2016-11-16 02:40:59]

Description was changed from

==========
bookmarks bar, unnecessary tooltip is seen for folders with small name.

BUG=635836
==========

to

==========
Bookmarks bar, unnecessary tooltip is seen for folders with small name.

BUG=635836
==========

[dhna, 2016-11-16 04:38:52]

corona10@gmail.com changed reviewers:
+ ellyjones@chromium.org, rsesek@chromium.org

[dhna, 2016-11-16 04:38:52]

PTAL

[dhna, 2016-11-16 05:40:07]

On 2016/11/16 04:38:52, dhna wrote:
> PTAL

Robert Sesek@
Elly Fong-Jones@

For reviewers I uploaded this CL to fix this issue (http://crbug.com/635836).
I also uploaded screenshots which are applied this patch.
I don't know which reviewer I should select so I added both of you.
Anyway please take a look and leave a comment.

[Robert Sesek, 2016-11-16 16:00:42]

Thanks for the patch! The code looks good, but do you think you could write a
unittest for makeButtonForNode:frame: so that this doesn't regress in the
future?

[dhna, 2016-11-16 21:02:05]

On 2016/11/16 16:00:42, Robert Sesek wrote:
> Thanks for the patch! The code looks good, but do you think you could write a
> unittest for makeButtonForNode:frame: so that this doesn't regress in the
> future?

Robert Sesek@
Sure I try to update it today.

[dhna, 2016-11-17 05:55:42]

On 2016/11/16 21:02:05, dhna wrote:
> On 2016/11/16 16:00:42, Robert Sesek wrote:
> > Thanks for the patch! The code looks good, but do you think you could write
a
> > unittest for makeButtonForNode:frame: so that this doesn't regress in the
> > future?
> 
> Robert Sesek@
> Sure I try to update it today.

Robert Sesek@
Today I tried to add unittest for makeButtonForNode:frame but failed.
Because of I'm not familiar with Cocoa unittest.
So can I get some guide or leave it for new issue or crbug? (i add it for
comment.)

Please take a look.

[dhna, 2016-11-17 05:56:40]

On 2016/11/17 05:55:42, dhna wrote:
> On 2016/11/16 21:02:05, dhna wrote:
> > On 2016/11/16 16:00:42, Robert Sesek wrote:
> > > Thanks for the patch! The code looks good, but do you think you could
write
> a
> > > unittest for makeButtonForNode:frame: so that this doesn't regress in the
> > > future?
> > 
> > Robert Sesek@
> > Sure I try to update it today.
> 
> Robert Sesek@
> Today I tried to add unittest for makeButtonForNode:frame but failed.
> Because of I'm not familiar with Cocoa unittest.
> So can I get some guide or leave it for new issue or crbug? (i add it for
> comment.)
> 
> Please take a look.

Robert Sesek@
Today I tried to add unittest for makeButtonForNode:frame but failed.
Because of I'm not familiar with Cocoa unittest.
So can I get some guide or leave it for new issue on crbug? (I add it for
comment.)

Please take a look.

[Robert Sesek, 2016-11-17 17:03:43]

On 2016/11/17 05:56:40, dhna wrote:
> On 2016/11/17 05:55:42, dhna wrote:
> > On 2016/11/16 21:02:05, dhna wrote:
> > > On 2016/11/16 16:00:42, Robert Sesek wrote:
> > > > Thanks for the patch! The code looks good, but do you think you could
> write
> > a
> > > > unittest for makeButtonForNode:frame: so that this doesn't regress in
the
> > > > future?
> > > 
> > > Robert Sesek@
> > > Sure I try to update it today.
> > 
> > Robert Sesek@
> > Today I tried to add unittest for makeButtonForNode:frame but failed.
> > Because of I'm not familiar with Cocoa unittest.
> > So can I get some guide or leave it for new issue or crbug? (i add it for
> > comment.)
> > 
> > Please take a look.
> 
> Robert Sesek@
> Today I tried to add unittest for makeButtonForNode:frame but failed.
> Because of I'm not familiar with Cocoa unittest.
> So can I get some guide or leave it for new issue on crbug? (I add it for
> comment.)

In my experience, bugs for simply writing tests will never be addressed, so
there's not much incentive to filing one. But I can definitely give you some
guidance.

The test file is
https://cs.chromium.org/chromium/src/chrome/browser/ui/cocoa/bookmarks/bookma....
The RemoveLastButtonOtherBookmarks test shows how to work with the "Other
Bookmarks" folder, as does ControllerForNode below it. Those would be good tests
to template this new one on.

For the test, I'd do something like this:

- Create a new test for the tooltips (e.g., FolderTooltips)
- Use the BookmarkModel to get the other_node()
- Add two folders to that node, one with a very long name and one with a shorter
name
- Force the otherBookmarksButton to open the menu, so that the
BookmarkBarFolderController actually generates the node and calls
makeButtonForNode:frame:
- Access the array of buttons
- Check that the short-title'd item has no tooltip
- Check that the long-title'd item does have the correct tooltip

[dhna, 2016-11-17 23:21:24]

On 2016/11/17 17:03:43, Robert Sesek wrote:
> On 2016/11/17 05:56:40, dhna wrote:
> > On 2016/11/17 05:55:42, dhna wrote:
> > > On 2016/11/16 21:02:05, dhna wrote:
> > > > On 2016/11/16 16:00:42, Robert Sesek wrote:
> > > > > Thanks for the patch! The code looks good, but do you think you could
> > write
> > > a
> > > > > unittest for makeButtonForNode:frame: so that this doesn't regress in
> the
> > > > > future?
> > > > 
> > > > Robert Sesek@
> > > > Sure I try to update it today.
> > > 
> > > Robert Sesek@
> > > Today I tried to add unittest for makeButtonForNode:frame but failed.
> > > Because of I'm not familiar with Cocoa unittest.
> > > So can I get some guide or leave it for new issue or crbug? (i add it for
> > > comment.)
> > > 
> > > Please take a look.
> > 
> > Robert Sesek@
> > Today I tried to add unittest for makeButtonForNode:frame but failed.
> > Because of I'm not familiar with Cocoa unittest.
> > So can I get some guide or leave it for new issue on crbug? (I add it for
> > comment.)
> 
> In my experience, bugs for simply writing tests will never be addressed, so
> there's not much incentive to filing one. But I can definitely give you some
> guidance.
> 
> The test file is
>
https://cs.chromium.org/chromium/src/chrome/browser/ui/cocoa/bookmarks/bookma....
> The RemoveLastButtonOtherBookmarks test shows how to work with the "Other
> Bookmarks" folder, as does ControllerForNode below it. Those would be good
tests
> to template this new one on.
> 
> For the test, I'd do something like this:
> 
> - Create a new test for the tooltips (e.g., FolderTooltips)
> - Use the BookmarkModel to get the other_node()
> - Add two folders to that node, one with a very long name and one with a
shorter
> name
> - Force the otherBookmarksButton to open the menu, so that the
> BookmarkBarFolderController actually generates the node and calls
> makeButtonForNode:frame:
> - Access the array of buttons
> - Check that the short-title'd item has no tooltip
> - Check that the long-title'd item does have the correct tooltip

Robert Sesek@

Thanks for your kind and wonderful guide .
I could add unit test code for this fix.
And thank you for understanding that I'm not familiar with Cocoa programming.
Please take a look.
Thanks~!

[dhna, 2016-11-19 00:48:24]

On 2016/11/17 23:21:24, dhna wrote:
> On 2016/11/17 17:03:43, Robert Sesek wrote:
> > On 2016/11/17 05:56:40, dhna wrote:
> > > On 2016/11/17 05:55:42, dhna wrote:
> > > > On 2016/11/16 21:02:05, dhna wrote:
> > > > > On 2016/11/16 16:00:42, Robert Sesek wrote:
> > > > > > Thanks for the patch! The code looks good, but do you think you
could
> > > write
> > > > a
> > > > > > unittest for makeButtonForNode:frame: so that this doesn't regress
in
> > the
> > > > > > future?
> > > > > 
> > > > > Robert Sesek@
> > > > > Sure I try to update it today.
> > > > 
> > > > Robert Sesek@
> > > > Today I tried to add unittest for makeButtonForNode:frame but failed.
> > > > Because of I'm not familiar with Cocoa unittest.
> > > > So can I get some guide or leave it for new issue or crbug? (i add it
for
> > > > comment.)
> > > > 
> > > > Please take a look.
> > > 
> > > Robert Sesek@
> > > Today I tried to add unittest for makeButtonForNode:frame but failed.
> > > Because of I'm not familiar with Cocoa unittest.
> > > So can I get some guide or leave it for new issue on crbug? (I add it for
> > > comment.)
> > 
> > In my experience, bugs for simply writing tests will never be addressed, so
> > there's not much incentive to filing one. But I can definitely give you some
> > guidance.
> > 
> > The test file is
> >
>
https://cs.chromium.org/chromium/src/chrome/browser/ui/cocoa/bookmarks/bookma....
> > The RemoveLastButtonOtherBookmarks test shows how to work with the "Other
> > Bookmarks" folder, as does ControllerForNode below it. Those would be good
> tests
> > to template this new one on.
> > 
> > For the test, I'd do something like this:
> > 
> > - Create a new test for the tooltips (e.g., FolderTooltips)
> > - Use the BookmarkModel to get the other_node()
> > - Add two folders to that node, one with a very long name and one with a
> shorter
> > name
> > - Force the otherBookmarksButton to open the menu, so that the
> > BookmarkBarFolderController actually generates the node and calls
> > makeButtonForNode:frame:
> > - Access the array of buttons
> > - Check that the short-title'd item has no tooltip
> > - Check that the long-title'd item does have the correct tooltip
> 
> Robert Sesek@
> 
> Thanks for your kind and wonderful guide .
> I could add unit test code for this fix.
> And thank you for understanding that I'm not familiar with Cocoa programming.
> Please take a look.
> Thanks~!

Robert Sesek@
Can you take a look if you have a time?

[Robert Sesek, 2016-11-21 16:17:51]

Sorry, I missed your message last week. LGTM, thanks for adding a test.

[dhna, 2016-11-21 16:21:27]

On 2016/11/21 16:17:51, Robert Sesek wrote:
> Sorry, I missed your message last week. LGTM, thanks for adding a test.

Rober Sesek@
Thanks to review my CL. :-)

[dhna, 2016-11-21 16:21:35]

The CQ bit was checked by corona10@gmail.com

[commit-bot: I haz the power, 2016-11-21 16:22:04]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-21 16:38:57]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1479745295686770,
"parent_rev": "f67f96e160f4593ae9c980855f42984ef5dff1bf", "commit_rev":
"4c111739438d09aa0cc818280dfd0a35d738bf59"}

[commit-bot: I haz the power, 2016-11-21 16:39:22]

Description was changed from

==========
Bookmarks bar, unnecessary tooltip is seen for folders with small name.

BUG=635836
==========

to

==========
Bookmarks bar, unnecessary tooltip is seen for folders with small name.

BUG=635836
==========

[commit-bot: I haz the power, 2016-11-21 16:39:23]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-11-21 16:42:54]

Description was changed from

==========
Bookmarks bar, unnecessary tooltip is seen for folders with small name.

BUG=635836
==========

to

==========
Bookmarks bar, unnecessary tooltip is seen for folders with small name.

BUG=635836

Committed: https://crrev.com/efa1d76a70dfec345ff89d2ca978a46029c51e7e
Cr-Commit-Position: refs/heads/master@{#433563}
==========

[commit-bot: I haz the power, 2016-11-21 16:42:55]

Patchset 5 (id:??) landed as
https://crrev.com/efa1d76a70dfec345ff89d2ca978a46029c51e7e
Cr-Commit-Position: refs/heads/master@{#433563}