Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7277089:
Display a tooltip when mouse hovers over a folder in the bookmark bar. (Closed)
|
Created:
9 years, 5 months ago by PhilBeaudoin Modified:
9 years, 5 months ago CC:
chromium-reviews Base URL:
http://src.chromium.org/svn/trunk/src/ Visibility:
Public. |
DescriptionDisplay a tooltip when mouse hovers over a folder in the bookmark bar.
The tooltip is only displayed if the folder label is too wide for button which causes ellipsis to be displayed.
Views specific.
BUG=83838
TEST=Create a new folder with a long name in the bookmark bar and verifies the tooltip is displayed.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=91758
Patch Set 1 #
Total comments: 6
Patch Set 2 : '' #
Messages
Total messages: 18 (0 generated)
This is my first CL (as a Googler). Thanks for the review!
http://codereview.chromium.org/7277089/diff/1/chrome/browser/ui/views/bookmar... File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (right): http://codereview.chromium.org/7277089/diff/1/chrome/browser/ui/views/bookmar... chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:198: bool GetTooltipText(const gfx::Point& p, std::wstring* tooltip) { Please use virtual and OVERRIDE here (like |IsTriggerableEvent()| below).
http://codereview.chromium.org/7277089/diff/1/chrome/browser/ui/views/bookmar... File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (right): http://codereview.chromium.org/7277089/diff/1/chrome/browser/ui/views/bookmar... chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:199: gfx::Point location(p); It doesn't look 199 and 200 are needed. http://codereview.chromium.org/7277089/diff/1/chrome/browser/ui/views/bookmar... chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:201: if (text_size_.width() > GetTextBounds().width()) { no {}
All issues addressed. http://codereview.chromium.org/7277089/diff/1/chrome/browser/ui/views/bookmar... File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (right): http://codereview.chromium.org/7277089/diff/1/chrome/browser/ui/views/bookmar... chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:198: bool GetTooltipText(const gfx::Point& p, std::wstring* tooltip) { On 2011/07/07 19:58:56, asvitkine_ wrote: > Please use virtual and OVERRIDE here (like |IsTriggerableEvent()| below). Done. http://codereview.chromium.org/7277089/diff/1/chrome/browser/ui/views/bookmar... chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:199: gfx::Point location(p); On 2011/07/07 20:07:16, sky wrote: > It doesn't look 199 and 200 are needed. Done. http://codereview.chromium.org/7277089/diff/1/chrome/browser/ui/views/bookmar... chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:201: if (text_size_.width() > GetTextBounds().width()) { On 2011/07/07 20:07:16, sky wrote: > no {} Done.
LGTM
LGTM
On 2011/07/07 20:31:48, asvitkine_ wrote: > LGTM Great! Can I get one of you to commit?
Commit bot is on it. ;)
Change committed as 91758
This doesn't compile on linux and was backed out. See http://build.chromium.org/p/chromium/builders/Linux%20Builder%20%28ChromiumOS... for one. asvitkine, next time make sure you run a try job first. -Scott
My bad, I was under the impression that the commit bot runs try jobs. On Thursday, July 7, 2011, <sky@chromium.org> wrote: > This doesn't compile on linux and was backed out. See > http://build.chromium.org/p/chromium/builders/Linux%20Builder%20%28ChromiumOS... > for one. asvitkine, next time make sure you run a try job first. > > -Scott > > http://codereview.chromium.org/7277089/ >
On 2011/07/07 22:13:12, asvitkine_ wrote: > My bad, I was under the impression that the commit bot runs try jobs. > > On Thursday, July 7, 2011, <mailto:sky@chromium.org> wrote: > > This doesn't compile on linux and was backed out. See > > > http://build.chromium.org/p/chromium/builders/Linux%2520Builder%2520%2528Chro... > > for one. asvitkine, next time make sure you run a try job first. > > > > -Scott > > > > http://codereview.chromium.org/7277089/ > > I got this in my inbox: try success for 'commit-bot: 7277089-4004' try job on linux @ r91743 http://build.chromium.org/p/tryserver.chromium/ You are awesome! Try succeeded! http://build.chromium.org/p/tryserver.chromium/builders/linux/builds/35766 slave: vm163-m4 (Same for windows and mac.)
It's the linux_view and chromeos bots that didn't like the patch. -Scott On Thu, Jul 7, 2011 at 3:14 PM, <philippe.beaudoin@gmail.com> wrote: > On 2011/07/07 22:13:12, asvitkine_ wrote: >> >> My bad, I was under the impression that the commit bot runs try jobs. > >> On Thursday, July 7, 2011, <mailto:sky@chromium.org> wrote: >> > This doesn't compile on linux and was backed out. See >> > > > http://build.chromium.org/p/chromium/builders/Linux%2520Builder%2520%2528Chro... >> >> > for one. asvitkine, next time make sure you run a try job first. >> > >> > -Scott >> > >> > http://codereview.chromium.org/7277089/ >> > > > I got this in my inbox: > > try success for 'commit-bot: 7277089-4004' try job on linux @ r91743 > http://build.chromium.org/p/tryserver.chromium/ > > You are awesome! Try succeeded! > > http://build.chromium.org/p/tryserver.chromium/builders/linux/builds/35766 > slave: vm163-m4 > > (Same for windows and mac.) > > http://codereview.chromium.org/7277089/ >
I would have thought your cl would trigger try jobs on those bots. Did you explicitly specify which bots to run on? -Scott On Thu, Jul 7, 2011 at 3:20 PM, Scott Violet <sky@chromium.org> wrote: > It's the linux_view and chromeos bots that didn't like the patch. > > -Scott > > On Thu, Jul 7, 2011 at 3:14 PM, <philippe.beaudoin@gmail.com> wrote: >> On 2011/07/07 22:13:12, asvitkine_ wrote: >>> >>> My bad, I was under the impression that the commit bot runs try jobs. >> >>> On Thursday, July 7, 2011, <mailto:sky@chromium.org> wrote: >>> > This doesn't compile on linux and was backed out. See >>> > >> >> http://build.chromium.org/p/chromium/builders/Linux%2520Builder%2520%2528Chro... >>> >>> > for one. asvitkine, next time make sure you run a try job first. >>> > >>> > -Scott >>> > >>> > http://codereview.chromium.org/7277089/ >>> > >> >> I got this in my inbox: >> >> try success for 'commit-bot: 7277089-4004' try job on linux @ r91743 >> http://build.chromium.org/p/tryserver.chromium/ >> >> You are awesome! Try succeeded! >> >> http://build.chromium.org/p/tryserver.chromium/builders/linux/builds/35766 >> slave: vm163-m4 >> >> (Same for windows and mac.) >> >> http://codereview.chromium.org/7277089/ >> >
I just placed it in the commit queue by checking the commit checkbox in Rietveld. No idea why it didn't start the linux_view and chromeos trybots. Maybe this should be investigated? Philippe On Thu, Jul 7, 2011 at 10:21 PM, Scott Violet <sky@chromium.org> wrote: > I would have thought your cl would trigger try jobs on those bots. Did > you explicitly specify which bots to run on? > > -Scott > > On Thu, Jul 7, 2011 at 3:20 PM, Scott Violet <sky@chromium.org> wrote: > > It's the linux_view and chromeos bots that didn't like the patch. > > > > -Scott > > > > On Thu, Jul 7, 2011 at 3:14 PM, <philippe.beaudoin@gmail.com> wrote: > >> On 2011/07/07 22:13:12, asvitkine_ wrote: > >>> > >>> My bad, I was under the impression that the commit bot runs try jobs. > >> > >>> On Thursday, July 7, 2011, <mailto:sky@chromium.org> wrote: > >>> > This doesn't compile on linux and was backed out. See > >>> > > >> > >> > http://build.chromium.org/p/chromium/builders/Linux%2520Builder%2520%2528Chro... > >>> > >>> > for one. asvitkine, next time make sure you run a try job first. > >>> > > >>> > -Scott > >>> > > >>> > http://codereview.chromium.org/7277089/ > >>> > > >> > >> I got this in my inbox: > >> > >> try success for 'commit-bot: 7277089-4004' try job on linux @ r91743 > >> http://build.chromium.org/p/tryserver.chromium/ > >> > >> You are awesome! Try succeeded! > >> > >> > http://build.chromium.org/p/tryserver.chromium/builders/linux/builds/35766 > >> slave: vm163-m4 > >> > >> (Same for windows and mac.) > >> > >> http://codereview.chromium.org/7277089/ > >> > > >
I guess those aren't run by default by the commit bot. :\ On Thu, Jul 7, 2011 at 7:41 PM, Philippe Beaudoin <philippe.beaudoin@gmail.com> wrote: > I just placed it in the commit queue by checking the commit checkbox in > Rietveld. No idea why it didn't start the linux_view and chromeos trybots. > Maybe this should be investigated? > Philippe > On Thu, Jul 7, 2011 at 10:21 PM, Scott Violet <sky@chromium.org> wrote: >> >> I would have thought your cl would trigger try jobs on those bots. Did >> you explicitly specify which bots to run on? >> >> -Scott >> >> On Thu, Jul 7, 2011 at 3:20 PM, Scott Violet <sky@chromium.org> wrote: >> > It's the linux_view and chromeos bots that didn't like the patch. >> > >> > -Scott >> > >> > On Thu, Jul 7, 2011 at 3:14 PM, <philippe.beaudoin@gmail.com> wrote: >> >> On 2011/07/07 22:13:12, asvitkine_ wrote: >> >>> >> >>> My bad, I was under the impression that the commit bot runs try jobs. >> >> >> >>> On Thursday, July 7, 2011, <mailto:sky@chromium.org> wrote: >> >>> > This doesn't compile on linux and was backed out. See >> >>> > >> >> >> >> >> >> http://build.chromium.org/p/chromium/builders/Linux%2520Builder%2520%2528Chro... >> >>> >> >>> > for one. asvitkine, next time make sure you run a try job first. >> >>> > >> >>> > -Scott >> >>> > >> >>> > http://codereview.chromium.org/7277089/ >> >>> > >> >> >> >> I got this in my inbox: >> >> >> >> try success for 'commit-bot: 7277089-4004' try job on linux @ r91743 >> >> http://build.chromium.org/p/tryserver.chromium/ >> >> >> >> You are awesome! Try succeeded! >> >> >> >> >> >> http://build.chromium.org/p/tryserver.chromium/builders/linux/builds/35766 >> >> slave: vm163-m4 >> >> >> >> (Same for windows and mac.) >> >> >> >> http://codereview.chromium.org/7277089/ >> >> >> > > >
Marc-Antoine would know for sure. I would have thought we use the same algorithm to decide what bots to run as we do when you execute 'git try'. -Scott On Thu, Jul 7, 2011 at 4:55 PM, Alexei Svitkine <asvitkine@chromium.org> wrote: > I guess those aren't run by default by the commit bot. :\ > > On Thu, Jul 7, 2011 at 7:41 PM, Philippe Beaudoin > <philippe.beaudoin@gmail.com> wrote: >> I just placed it in the commit queue by checking the commit checkbox in >> Rietveld. No idea why it didn't start the linux_view and chromeos trybots. >> Maybe this should be investigated? >> Philippe >> On Thu, Jul 7, 2011 at 10:21 PM, Scott Violet <sky@chromium.org> wrote: >>> >>> I would have thought your cl would trigger try jobs on those bots. Did >>> you explicitly specify which bots to run on? >>> >>> -Scott >>> >>> On Thu, Jul 7, 2011 at 3:20 PM, Scott Violet <sky@chromium.org> wrote: >>> > It's the linux_view and chromeos bots that didn't like the patch. >>> > >>> > -Scott >>> > >>> > On Thu, Jul 7, 2011 at 3:14 PM, <philippe.beaudoin@gmail.com> wrote: >>> >> On 2011/07/07 22:13:12, asvitkine_ wrote: >>> >>> >>> >>> My bad, I was under the impression that the commit bot runs try jobs. >>> >> >>> >>> On Thursday, July 7, 2011, <mailto:sky@chromium.org> wrote: >>> >>> > This doesn't compile on linux and was backed out. See >>> >>> > >>> >> >>> >> >>> >> http://build.chromium.org/p/chromium/builders/Linux%2520Builder%2520%2528Chro... >>> >>> >>> >>> > for one. asvitkine, next time make sure you run a try job first. >>> >>> > >>> >>> > -Scott >>> >>> > >>> >>> > http://codereview.chromium.org/7277089/ >>> >>> > >>> >> >>> >> I got this in my inbox: >>> >> >>> >> try success for 'commit-bot: 7277089-4004' try job on linux @ r91743 >>> >> http://build.chromium.org/p/tryserver.chromium/ >>> >> >>> >> You are awesome! Try succeeded! >>> >> >>> >> >>> >> http://build.chromium.org/p/tryserver.chromium/builders/linux/builds/35766 >>> >> slave: vm163-m4 >>> >> >>> >> (Same for windows and mac.) >>> >> >>> >> http://codereview.chromium.org/7277089/ >>> >> >>> > >> >> >
Marc-Antoine is on vacation right now, so we won't hear from him for a while... On Thu, Jul 7, 2011 at 7:58 PM, Scott Violet <sky@chromium.org> wrote: > Marc-Antoine would know for sure. I would have thought we use the same > algorithm to decide what bots to run as we do when you execute 'git > try'. > > -Scott > > On Thu, Jul 7, 2011 at 4:55 PM, Alexei Svitkine <asvitkine@chromium.org> wrote: >> I guess those aren't run by default by the commit bot. :\ >> >> On Thu, Jul 7, 2011 at 7:41 PM, Philippe Beaudoin >> <philippe.beaudoin@gmail.com> wrote: >>> I just placed it in the commit queue by checking the commit checkbox in >>> Rietveld. No idea why it didn't start the linux_view and chromeos trybots. >>> Maybe this should be investigated? >>> Philippe >>> On Thu, Jul 7, 2011 at 10:21 PM, Scott Violet <sky@chromium.org> wrote: >>>> >>>> I would have thought your cl would trigger try jobs on those bots. Did >>>> you explicitly specify which bots to run on? >>>> >>>> -Scott >>>> >>>> On Thu, Jul 7, 2011 at 3:20 PM, Scott Violet <sky@chromium.org> wrote: >>>> > It's the linux_view and chromeos bots that didn't like the patch. >>>> > >>>> > -Scott >>>> > >>>> > On Thu, Jul 7, 2011 at 3:14 PM, <philippe.beaudoin@gmail.com> wrote: >>>> >> On 2011/07/07 22:13:12, asvitkine_ wrote: >>>> >>> >>>> >>> My bad, I was under the impression that the commit bot runs try jobs. >>>> >> >>>> >>> On Thursday, July 7, 2011, <mailto:sky@chromium.org> wrote: >>>> >>> > This doesn't compile on linux and was backed out. See >>>> >>> > >>>> >> >>>> >> >>>> >> http://build.chromium.org/p/chromium/builders/Linux%2520Builder%2520%2528Chro... >>>> >>> >>>> >>> > for one. asvitkine, next time make sure you run a try job first. >>>> >>> > >>>> >>> > -Scott >>>> >>> > >>>> >>> > http://codereview.chromium.org/7277089/ >>>> >>> > >>>> >> >>>> >> I got this in my inbox: >>>> >> >>>> >> try success for 'commit-bot: 7277089-4004' try job on linux @ r91743 >>>> >> http://build.chromium.org/p/tryserver.chromium/ >>>> >> >>>> >> You are awesome! Try succeeded! >>>> >> >>>> >> >>>> >> http://build.chromium.org/p/tryserver.chromium/builders/linux/builds/35766 >>>> >> slave: vm163-m4 >>>> >> >>>> >> (Same for windows and mac.) >>>> >> >>>> >> http://codereview.chromium.org/7277089/ >>>> >> >>>> > >>> >>> >> > |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
