Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
213833018:
Aura tooltips do not move on mouse move in case of many neighboring views with the same label (Closed)
|
Created:
6 years, 8 months ago by Mikus Modified:
6 years, 6 months ago Reviewers:
sky CC:
chromium-reviews, yusukes+watch_chromium.org, yukishiino+watch_chromium.org, tfarina, penghuang+watch_chromium.org, sievers+watch_chromium.org, jbauman+watch_chromium.org, nona+watch_chromium.org, darin-cc_chromium.org, kalyank, piman+watch_chromium.org, danakj+watch_chromium.org, James Su, ben+views_chromium.org, ben+corewm_chromium.org, miu+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionAura tooltips do not move on mouse move in case of many neighboring
views with the same tooltip label
BUG=Aura tooltips do not move on mouse move in case of many neighboring
views with the same tooltip label. This can be really uncomfortable
when working with lots of tabs with the same address on the
tab strip. Try creating ~30 startpage tabs and hover over one of them,
then move the cursor to others. The tooltip will just sit in the place
where it was first shown.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=277010
Patch Set 1 #Patch Set 2 : Fixed a problem with blinking tooltips on e.g. links. #Patch Set 3 : Remove view pointer from tooltip manager and replace it with bounds used for last displayed tooltip #Patch Set 4 : Fixed errors derived from manual patch transfer from Opera #Patch Set 5 : Write unit test for the case when a tooltip moves from one view to another with the same tooltip bu… #
Total comments: 2
Patch Set 6 : The test now uses TestTooltip as it should + formatting #Patch Set 7 : Remove redundant method from TooltipControllerTestHelper #Patch Set 8 : Update the tooltip according to void* uniqueness indicator, source view in case of views #Patch Set 9 : Sanity fixups #
Total comments: 19
Patch Set 10 : Review fixes & test adaptations #Patch Set 11 : Missed tooltip client property type fix #Patch Set 12 : Change the unique id of tooltip text from void** to void* #
Total comments: 4
Patch Set 13 : Code review fixes #Patch Set 14 : Do not set unique id along with tooltip text #
Total comments: 1
Patch Set 15 : UpdateTooltipId->SetTooltipId #Patch Set 16 : Fix Linux test TooltipControllerTest3 #
Messages
Total messages: 40 (0 generated)
can you tell us which reviewers are for which code? for example, I'm an owner in content, but so are ben/sky who are owners in content/browser
On 2014/04/01 16:19:46, jam wrote: > can you tell us which reviewers are for which code? for example, I'm an owner in > content, but so are ben/sky who are owners in content/browser and I'm not an OWNER in any of directories (nor worked on tooltips), so you probably don't need review from me.
On 2014/04/01 18:45:35, oshima wrote: > On 2014/04/01 16:19:46, jam wrote: > > can you tell us which reviewers are for which code? for example, I'm an owner > in > > content, but so are ben/sky who are owners in content/browser > > and I'm not an OWNER in any of directories (nor worked on tooltips), so you > probably don't need review from me. True, but you were in git history of some files. Anyways, I'll just remove you from the review as per your request.
On 2014/04/02 10:06:15, Mikus wrote: > On 2014/04/01 18:45:35, oshima wrote: > > On 2014/04/01 16:19:46, jam wrote: > > > can you tell us which reviewers are for which code? for example, I'm an > owner > > in > > > content, but so are ben/sky who are owners in content/browser > > > > and I'm not an OWNER in any of directories (nor worked on tooltips), so you > > probably don't need review from me. > > True, but you were in git history of some files. Anyways, I'll just remove you > from > the review as per your request. Selected reviewers are: render_widget_host_view_aura.cc - ben tooltip_controller.h/cc - sky tooltip_manager_aura.h/cc - sky tooltip_client.h - ben But I've added extra persons from git history, it's common in Opera, I have yet to learn Chromium's ways as it turns out ;)
Can you write a test here?
On 2014/04/02 15:05:06, sky wrote: > Can you write a test here? I think so. Will do that in next patch.
On 2014/04/09 13:07:33, Mikus wrote: > On 2014/04/02 15:05:06, sky wrote: > > Can you write a test here? > > I think so. Will do that in next patch. Done. I will further enhance it by introducing nested views.
https://codereview.chromium.org/213833018/diff/80001/ui/views/corewm/tooltip.h File ui/views/corewm/tooltip.h (right): https://codereview.chromium.org/213833018/diff/80001/ui/views/corewm/tooltip.... ui/views/corewm/tooltip.h:49: virtual gfx::Point GetTooltipPosition() = 0; You shouldn't have to expose this to test the fix. You should be able to change TestTooltip to cache the location. https://codereview.chromium.org/213833018/diff/80001/ui/views/widget/tooltip_... File ui/views/widget/tooltip_manager_aura.cc (right): https://codereview.chromium.org/213833018/diff/80001/ui/views/widget/tooltip_... ui/views/widget/tooltip_manager_aura.cc:137: gfx::Point target_rect_origin_in_screen; This is a pain for each site to have to deal with. I think a better approach is for SetTooltipText to take an additional void* that is used for uniqueness. Views would pass in the target.
I'm also trimming the review list to just me. In the future pick one reviewer otherwise you run the risk of all reviewers waiting for the others to respond.
On 2014/04/10 16:12:08, sky wrote: > https://codereview.chromium.org/213833018/diff/80001/ui/views/corewm/tooltip.h > File ui/views/corewm/tooltip.h (right): > > https://codereview.chromium.org/213833018/diff/80001/ui/views/corewm/tooltip.... > ui/views/corewm/tooltip.h:49: virtual gfx::Point GetTooltipPosition() = 0; > You shouldn't have to expose this to test the fix. You should be able to change > TestTooltip to cache the location. > OK > https://codereview.chromium.org/213833018/diff/80001/ui/views/widget/tooltip_... > File ui/views/widget/tooltip_manager_aura.cc (right): > > https://codereview.chromium.org/213833018/diff/80001/ui/views/widget/tooltip_... > ui/views/widget/tooltip_manager_aura.cc:137: gfx::Point > target_rect_origin_in_screen; > This is a pain for each site to have to deal with. I think a better approach is > for SetTooltipText to take an additional void* that is used for uniqueness. > Views would pass in the target. I don't quite understand. Pass in the view and what? I cannot check if it contains another view in toolbar_controller, could you please explain a little bit more?
On Fri, Apr 11, 2014 at 3:06 AM, <mboc@opera.com> wrote: > On 2014/04/10 16:12:08, sky wrote: >> >> >> https://codereview.chromium.org/213833018/diff/80001/ui/views/corewm/tooltip.h >> File ui/views/corewm/tooltip.h (right): > > > > https://codereview.chromium.org/213833018/diff/80001/ui/views/corewm/tooltip.... >> >> ui/views/corewm/tooltip.h:49: virtual gfx::Point GetTooltipPosition() = 0; >> You shouldn't have to expose this to test the fix. You should be able to > > change >> >> TestTooltip to cache the location. > > > > OK > > > > https://codereview.chromium.org/213833018/diff/80001/ui/views/widget/tooltip_... >> >> File ui/views/widget/tooltip_manager_aura.cc (right): > > > > https://codereview.chromium.org/213833018/diff/80001/ui/views/widget/tooltip_... >> >> ui/views/widget/tooltip_manager_aura.cc:137: gfx::Point >> target_rect_origin_in_screen; >> This is a pain for each site to have to deal with. I think a better >> approach > > is >> >> for SetTooltipText to take an additional void* that is used for >> uniqueness. >> Views would pass in the target. > > > I don't quite understand. Pass in the view and what? Make TooltipClient take a void* that is used for uniqueness. If the text is the same, but the void* changes, it treats it as though the text changed. -Scott > I cannot check if it > contains another view in toolbar_controller, could you please explain a > little > bit more? > > > https://codereview.chromium.org/213833018/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/04/14 14:03:10, sky wrote: > On Fri, Apr 11, 2014 at 3:06 AM, <mailto:mboc@opera.com> wrote: > > On 2014/04/10 16:12:08, sky wrote: > >> > >> > >> > https://codereview.chromium.org/213833018/diff/80001/ui/views/corewm/tooltip.h > >> File ui/views/corewm/tooltip.h (right): > > > > > > > > > https://codereview.chromium.org/213833018/diff/80001/ui/views/corewm/tooltip.... > >> > >> ui/views/corewm/tooltip.h:49: virtual gfx::Point GetTooltipPosition() = 0; > >> You shouldn't have to expose this to test the fix. You should be able to > > > > change > >> > >> TestTooltip to cache the location. > > > > > > > > OK > > > > > > > > > https://codereview.chromium.org/213833018/diff/80001/ui/views/widget/tooltip_... > >> > >> File ui/views/widget/tooltip_manager_aura.cc (right): > > > > > > > > > https://codereview.chromium.org/213833018/diff/80001/ui/views/widget/tooltip_... > >> > >> ui/views/widget/tooltip_manager_aura.cc:137: gfx::Point > >> target_rect_origin_in_screen; > >> This is a pain for each site to have to deal with. I think a better > >> approach > > > > is > >> > >> for SetTooltipText to take an additional void* that is used for > >> uniqueness. > >> Views would pass in the target. > > > > > > I don't quite understand. Pass in the view and what? > > Make TooltipClient take a void* that is used for uniqueness. If the > text is the same, but the void* changes, it treats it as though the > text changed. > Well I know what you meant, but then if we have for example tab, it contains a label and image and so on, the tooltip is going to jump around a lot. > -Scott > > > I cannot check if it > > contains another view in toolbar_controller, could you please explain a > > little > > bit more? > > > > > > https://codereview.chromium.org/213833018/ > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
On Mon, Apr 14, 2014 at 7:24 AM, <mboc@opera.com> wrote: > On 2014/04/14 14:03:10, sky wrote: > >> On Fri, Apr 11, 2014 at 3:06 AM, <mailto:mboc@opera.com> wrote: >> > On 2014/04/10 16:12:08, sky wrote: >> >> >> >> >> >> >> >> https://codereview.chromium.org/213833018/diff/80001/ui/views/corewm/tooltip.h >> >> File ui/views/corewm/tooltip.h (right): >> > >> > >> > >> > > > > https://codereview.chromium.org/213833018/diff/80001/ui/views/corewm/tooltip.... >> >> >> >> >> ui/views/corewm/tooltip.h:49: virtual gfx::Point GetTooltipPosition() = >> >> 0; >> >> You shouldn't have to expose this to test the fix. You should be able >> >> to >> > >> > change >> >> >> >> TestTooltip to cache the location. >> > >> > >> > >> > OK >> > >> > >> > >> > > > > https://codereview.chromium.org/213833018/diff/80001/ui/views/widget/tooltip_... >> >> >> >> >> File ui/views/widget/tooltip_manager_aura.cc (right): >> > >> > >> > >> > > > > https://codereview.chromium.org/213833018/diff/80001/ui/views/widget/tooltip_... >> >> >> >> >> ui/views/widget/tooltip_manager_aura.cc:137: gfx::Point >> >> target_rect_origin_in_screen; >> >> This is a pain for each site to have to deal with. I think a better >> >> approach >> > >> > is >> >> >> >> for SetTooltipText to take an additional void* that is used for >> >> uniqueness. >> >> Views would pass in the target. >> > >> > >> > I don't quite understand. Pass in the view and what? > > >> Make TooltipClient take a void* that is used for uniqueness. If the >> text is the same, but the void* changes, it treats it as though the >> text changed. > > > > Well I know what you meant, but then if we have for example tab, it contains > a > label and image and so on, the tooltip is going to jump around a lot. In that case the subviews should not have the tooltip, only the parent (tab). I believe this is the case with the tab class in chrome now. -Scott > >> -Scott > > >> > I cannot check if it >> > contains another view in toolbar_controller, could you please explain a >> > little >> > bit more? >> > >> > >> > https://codereview.chromium.org/213833018/ > > >> To unsubscribe from this group and stop receiving emails from it, send an > > email >> >> to mailto:chromium-reviews+unsubscribe@chromium.org. > > > > > https://codereview.chromium.org/213833018/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/04/14 15:37:23, sky wrote: > On Mon, Apr 14, 2014 at 7:24 AM, <mailto:mboc@opera.com> wrote: > > On 2014/04/14 14:03:10, sky wrote: > > > >> On Fri, Apr 11, 2014 at 3:06 AM, <mailto:mboc@opera.com> wrote: > >> > On 2014/04/10 16:12:08, sky wrote: > >> >> > >> >> > >> >> > >> > >> > https://codereview.chromium.org/213833018/diff/80001/ui/views/corewm/tooltip.h > >> >> File ui/views/corewm/tooltip.h (right): > >> > > >> > > >> > > >> > > > > > > > > https://codereview.chromium.org/213833018/diff/80001/ui/views/corewm/tooltip.... > >> > >> >> > >> >> ui/views/corewm/tooltip.h:49: virtual gfx::Point GetTooltipPosition() = > >> >> 0; > >> >> You shouldn't have to expose this to test the fix. You should be able > >> >> to > >> > > >> > change > >> >> > >> >> TestTooltip to cache the location. > >> > > >> > > >> > > >> > OK > >> > > >> > > >> > > >> > > > > > > > > https://codereview.chromium.org/213833018/diff/80001/ui/views/widget/tooltip_... > >> > >> >> > >> >> File ui/views/widget/tooltip_manager_aura.cc (right): > >> > > >> > > >> > > >> > > > > > > > > https://codereview.chromium.org/213833018/diff/80001/ui/views/widget/tooltip_... > >> > >> >> > >> >> ui/views/widget/tooltip_manager_aura.cc:137: gfx::Point > >> >> target_rect_origin_in_screen; > >> >> This is a pain for each site to have to deal with. I think a better > >> >> approach > >> > > >> > is > >> >> > >> >> for SetTooltipText to take an additional void* that is used for > >> >> uniqueness. > >> >> Views would pass in the target. > >> > > >> > > >> > I don't quite understand. Pass in the view and what? > > > > > >> Make TooltipClient take a void* that is used for uniqueness. If the > >> text is the same, but the void* changes, it treats it as though the > >> text changed. > > > > > > > > Well I know what you meant, but then if we have for example tab, it contains > > a > > label and image and so on, the tooltip is going to jump around a lot. > > In that case the subviews should not have the tooltip, only the parent > (tab). I believe this is the case with the tab class in chrome now. > > -Scott > I'll have to modify our (Opera) code first to get this running. Mikus > > > >> -Scott > > > > > >> > I cannot check if it > >> > contains another view in toolbar_controller, could you please explain a > >> > little > >> > bit more? > >> > > >> > > >> > https://codereview.chromium.org/213833018/ > > > > > >> To unsubscribe from this group and stop receiving emails from it, send an > > > > email > >> > >> to mailto:chromium-reviews+unsubscribe@chromium.org. > > > > > > > > > > https://codereview.chromium.org/213833018/ > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
Done!
https://codereview.chromium.org/213833018/diff/180001/ui/views/corewm/tooltip... File ui/views/corewm/tooltip_controller.cc (right): https://codereview.chromium.org/213833018/diff/180001/ui/views/corewm/tooltip... ui/views/corewm/tooltip_controller.cc:282: bool force = false; force->ids_differ https://codereview.chromium.org/213833018/diff/180001/ui/views/corewm/tooltip... File ui/views/corewm/tooltip_controller.h (right): https://codereview.chromium.org/213833018/diff/180001/ui/views/corewm/tooltip... ui/views/corewm/tooltip_controller.h:64: // text or the aura::Window). If |force| is true the tooltip will be updated There is no force here. https://codereview.chromium.org/213833018/diff/180001/ui/views/corewm/tooltip... File ui/views/corewm/tooltip_controller_unittest.cc (right): https://codereview.chromium.org/213833018/diff/180001/ui/views/corewm/tooltip... ui/views/corewm/tooltip_controller_unittest.cc:683: gfx::Point location() { return location_; } const gfx::Point& https://codereview.chromium.org/213833018/diff/180001/ui/views/widget/tooltip... File ui/views/widget/tooltip_manager_aura.cc (right): https://codereview.chromium.org/213833018/diff/180001/ui/views/widget/tooltip... ui/views/widget/tooltip_manager_aura.cc:14: #include "ui/views/view.h" not needed. https://codereview.chromium.org/213833018/diff/180001/ui/views/widget/tooltip... ui/views/widget/tooltip_manager_aura.cc:127: bool related = false; not needed. https://codereview.chromium.org/213833018/diff/180001/ui/views/widget/tooltip... File ui/views/widget/tooltip_manager_aura.h (right): https://codereview.chromium.org/213833018/diff/180001/ui/views/widget/tooltip... ui/views/widget/tooltip_manager_aura.h:11: #include "ui/gfx/rect.h" Seems like you don't need any of the changes you have done to the header (tooltip_unique_ is only used inside one place) https://codereview.chromium.org/213833018/diff/180001/ui/wm/public/tooltip_cl... File ui/wm/public/tooltip_client.cc (right): https://codereview.chromium.org/213833018/diff/180001/ui/wm/public/tooltip_cl... ui/wm/public/tooltip_client.cc:47: void** ptr = window->GetProperty(kTooltipUniqueKey); Seems like this implementation should be: return window->GetProperty(kTooltipUniqueKey); https://codereview.chromium.org/213833018/diff/180001/ui/wm/public/tooltip_cl... File ui/wm/public/tooltip_client.h (right): https://codereview.chromium.org/213833018/diff/180001/ui/wm/public/tooltip_cl... ui/wm/public/tooltip_client.h:41: void** tooltip_unique); Why does this need to be a void**? Why not void*? Also, document what it is. I suggest naming it id as well. https://codereview.chromium.org/213833018/diff/180001/ui/wm/public/tooltip_cl... ui/wm/public/tooltip_client.h:43: AURA_EXPORT const void* GetTooltipUnique(Window* window); GetTooltipId?
https://codereview.chromium.org/213833018/diff/180001/ui/views/corewm/tooltip... File ui/views/corewm/tooltip_controller.cc (right): https://codereview.chromium.org/213833018/diff/180001/ui/views/corewm/tooltip... ui/views/corewm/tooltip_controller.cc:282: bool force = false; On 2014/04/23 20:08:46, sky wrote: > force->ids_differ Done. https://codereview.chromium.org/213833018/diff/180001/ui/views/corewm/tooltip... File ui/views/corewm/tooltip_controller.h (right): https://codereview.chromium.org/213833018/diff/180001/ui/views/corewm/tooltip... ui/views/corewm/tooltip_controller.h:64: // text or the aura::Window). If |force| is true the tooltip will be updated On 2014/04/23 20:08:46, sky wrote: > There is no force here. Done. https://codereview.chromium.org/213833018/diff/180001/ui/views/corewm/tooltip... File ui/views/corewm/tooltip_controller_unittest.cc (right): https://codereview.chromium.org/213833018/diff/180001/ui/views/corewm/tooltip... ui/views/corewm/tooltip_controller_unittest.cc:683: gfx::Point location() { return location_; } On 2014/04/23 20:08:46, sky wrote: > const gfx::Point& Done. https://codereview.chromium.org/213833018/diff/180001/ui/views/widget/tooltip... File ui/views/widget/tooltip_manager_aura.cc (right): https://codereview.chromium.org/213833018/diff/180001/ui/views/widget/tooltip... ui/views/widget/tooltip_manager_aura.cc:14: #include "ui/views/view.h" On 2014/04/23 20:08:46, sky wrote: > not needed. Done. https://codereview.chromium.org/213833018/diff/180001/ui/views/widget/tooltip... ui/views/widget/tooltip_manager_aura.cc:127: bool related = false; On 2014/04/23 20:08:46, sky wrote: > not needed. Done. https://codereview.chromium.org/213833018/diff/180001/ui/views/widget/tooltip... File ui/views/widget/tooltip_manager_aura.h (right): https://codereview.chromium.org/213833018/diff/180001/ui/views/widget/tooltip... ui/views/widget/tooltip_manager_aura.h:11: #include "ui/gfx/rect.h" On 2014/04/23 20:08:46, sky wrote: > Seems like you don't need any of the changes you have done to the header > (tooltip_unique_ is only used inside one place) True, but I need tooltip_unique_ (actually tooltip_id_ now) so that the mechanism is the same as for tooltip_text_. https://codereview.chromium.org/213833018/diff/180001/ui/wm/public/tooltip_cl... File ui/wm/public/tooltip_client.cc (right): https://codereview.chromium.org/213833018/diff/180001/ui/wm/public/tooltip_cl... ui/wm/public/tooltip_client.cc:47: void** ptr = window->GetProperty(kTooltipUniqueKey); On 2014/04/23 20:08:46, sky wrote: > Seems like this implementation should be: > return window->GetProperty(kTooltipUniqueKey); Not really. It doesn't work in that case since we return a non-null pointer always (as it's a pointer to field in tooltip_manager_aura_). And it's always the same of course. https://codereview.chromium.org/213833018/diff/180001/ui/wm/public/tooltip_cl... File ui/wm/public/tooltip_client.h (right): https://codereview.chromium.org/213833018/diff/180001/ui/wm/public/tooltip_cl... ui/wm/public/tooltip_client.h:41: void** tooltip_unique); On 2014/04/23 20:08:46, sky wrote: > Why does this need to be a void**? Why not void*? Also, document what it is. I > suggest naming it id as well. I want to reference a void* field of tooltip_manager_aura, hence void** (just like it's done with tooltip_text_). Otherwise it's impossible (I cannot cast reinterpret_cast<void*>(*tooltip_id_) since *tooltip_id_ is an invalid indirection. I named it ID and documented it, though. https://codereview.chromium.org/213833018/diff/180001/ui/wm/public/tooltip_cl... ui/wm/public/tooltip_client.h:43: AURA_EXPORT const void* GetTooltipUnique(Window* window); On 2014/04/23 20:08:46, sky wrote: > GetTooltipId? Done.
https://codereview.chromium.org/213833018/diff/180001/ui/wm/public/tooltip_cl... File ui/wm/public/tooltip_client.h (right): https://codereview.chromium.org/213833018/diff/180001/ui/wm/public/tooltip_cl... ui/wm/public/tooltip_client.h:41: void** tooltip_unique); On 2014/04/24 08:39:56, Mikus wrote: > On 2014/04/23 20:08:46, sky wrote: > > Why does this need to be a void**? Why not void*? Also, document what it is. I > > suggest naming it id as well. > > I want to reference a void* field of tooltip_manager_aura, hence void** (just > like it's done with tooltip_text_). Otherwise it's impossible (I cannot cast > reinterpret_cast<void*>(*tooltip_id_) since *tooltip_id_ is an invalid > indirection. Just like you invoke SetTooltipText with a string*, not a string** this should take a void*. It should be responsible for recognizing when the *id changes. It should be told. In that case a void* is all you need. > > I named it ID and documented it, though.
On 2014/04/24 16:19:43, sky wrote: > https://codereview.chromium.org/213833018/diff/180001/ui/wm/public/tooltip_cl... > File ui/wm/public/tooltip_client.h (right): > > https://codereview.chromium.org/213833018/diff/180001/ui/wm/public/tooltip_cl... > ui/wm/public/tooltip_client.h:41: void** tooltip_unique); > On 2014/04/24 08:39:56, Mikus wrote: > > On 2014/04/23 20:08:46, sky wrote: > > > Why does this need to be a void**? Why not void*? Also, document what it is. > I > > > suggest naming it id as well. > > > > I want to reference a void* field of tooltip_manager_aura, hence void** (just > > like it's done with tooltip_text_). Otherwise it's impossible (I cannot cast > > reinterpret_cast<void*>(*tooltip_id_) since *tooltip_id_ is an invalid > > indirection. > > Just like you invoke SetTooltipText with a string*, not a string** this should > take a void*. It should be responsible for recognizing when the *id changes. It > should be told. In that case a void* is all you need. > > > > I named it ID and documented it, though. But you reference a view pointer, not a view itself. This is the basic difference between the string and view* case.
On 2014/04/29 11:23:05, Mikus wrote: > On 2014/04/24 16:19:43, sky wrote: > > > https://codereview.chromium.org/213833018/diff/180001/ui/wm/public/tooltip_cl... > > File ui/wm/public/tooltip_client.h (right): > > > > > https://codereview.chromium.org/213833018/diff/180001/ui/wm/public/tooltip_cl... > > ui/wm/public/tooltip_client.h:41: void** tooltip_unique); > > On 2014/04/24 08:39:56, Mikus wrote: > > > On 2014/04/23 20:08:46, sky wrote: > > > > Why does this need to be a void**? Why not void*? Also, document what it > is. > > I > > > > suggest naming it id as well. > > > > > > I want to reference a void* field of tooltip_manager_aura, hence void** > (just > > > like it's done with tooltip_text_). Otherwise it's impossible (I cannot cast > > > reinterpret_cast<void*>(*tooltip_id_) since *tooltip_id_ is an invalid > > > indirection. > > > > Just like you invoke SetTooltipText with a string*, not a string** this should > > take a void*. It should be responsible for recognizing when the *id changes. > It > > should be told. In that case a void* is all you need. > > > > > > I named it ID and documented it, though. > > But you reference a view pointer, not a view itself. This is the basic > difference between the string and view* case. I really don't get how you want to do this, setting the void* property to something else each time the field would otherwise change?
On 2014/04/29 11:24:15, Mikus wrote: > On 2014/04/29 11:23:05, Mikus wrote: > > On 2014/04/24 16:19:43, sky wrote: > > > > > > https://codereview.chromium.org/213833018/diff/180001/ui/wm/public/tooltip_cl... > > > File ui/wm/public/tooltip_client.h (right): > > > > > > > > > https://codereview.chromium.org/213833018/diff/180001/ui/wm/public/tooltip_cl... > > > ui/wm/public/tooltip_client.h:41: void** tooltip_unique); > > > On 2014/04/24 08:39:56, Mikus wrote: > > > > On 2014/04/23 20:08:46, sky wrote: > > > > > Why does this need to be a void**? Why not void*? Also, document what it > > is. > > > I > > > > > suggest naming it id as well. > > > > > > > > I want to reference a void* field of tooltip_manager_aura, hence void** > > (just > > > > like it's done with tooltip_text_). Otherwise it's impossible (I cannot > cast > > > > reinterpret_cast<void*>(*tooltip_id_) since *tooltip_id_ is an invalid > > > > indirection. > > > > > > Just like you invoke SetTooltipText with a string*, not a string** this > should > > > take a void*. It should be responsible for recognizing when the *id changes. > > It > > > should be told. In that case a void* is all you need. > > > > > > > > I named it ID and documented it, though. > > > > But you reference a view pointer, not a view itself. This is the basic > > difference between the string and view* case. > > I really don't get how you want to do this, setting the void* property to > something else each time the field would otherwise change? That's right. TooltipController::UpdateTooltip can check the void*s. If they differ, then it should reposition.
On 2014/04/29 16:05:16, sky wrote: Sorry, I had a rather long holiday. @sky I've just uploaded a patch for what you wanted.
https://codereview.chromium.org/213833018/diff/240001/ui/views/corewm/tooltip... File ui/views/corewm/tooltip_controller.cc (right): https://codereview.chromium.org/213833018/diff/240001/ui/views/corewm/tooltip... ui/views/corewm/tooltip_controller.cc:290: if (tooltip_id) { Why the if here? Can't you always compare tooltip_id_ and tooltip_id? https://codereview.chromium.org/213833018/diff/240001/ui/views/widget/tooltip... File ui/views/widget/tooltip_manager_aura.cc (left): https://codereview.chromium.org/213833018/diff/240001/ui/views/widget/tooltip... ui/views/widget/tooltip_manager_aura.cc:29: TooltipManagerAura::TooltipManagerAura(Widget* widget) : widget_(widget) { Style here is fine, leave it. https://codereview.chromium.org/213833018/diff/240001/ui/wm/public/tooltip_cl... File ui/wm/public/tooltip_client.cc (right): https://codereview.chromium.org/213833018/diff/240001/ui/wm/public/tooltip_cl... ui/wm/public/tooltip_client.cc:36: window->SetProperty(kTooltipIdKey, static_cast<void*>(NULL)); Is this really necessary? https://codereview.chromium.org/213833018/diff/240001/ui/wm/public/tooltip_cl... File ui/wm/public/tooltip_client.h (right): https://codereview.chromium.org/213833018/diff/240001/ui/wm/public/tooltip_cl... ui/wm/public/tooltip_client.h:39: // Sets the |tooltip_text| and |id| pointers as properties of the |window|. This is rather confusing. How about: Sets the text for the tooltip. The id is used to determine uniqueness when the text does not change. For example, if the tooltip text does not change, but the id does then the position of the tooltip is updated.
On 2014/06/02 15:39:46, sky wrote: > https://codereview.chromium.org/213833018/diff/240001/ui/views/corewm/tooltip... > File ui/views/corewm/tooltip_controller.cc (right): > > https://codereview.chromium.org/213833018/diff/240001/ui/views/corewm/tooltip... > ui/views/corewm/tooltip_controller.cc:290: if (tooltip_id) { > Why the if here? Can't you always compare tooltip_id_ and tooltip_id? > > https://codereview.chromium.org/213833018/diff/240001/ui/views/widget/tooltip... > File ui/views/widget/tooltip_manager_aura.cc (left): > > https://codereview.chromium.org/213833018/diff/240001/ui/views/widget/tooltip... > ui/views/widget/tooltip_manager_aura.cc:29: > TooltipManagerAura::TooltipManagerAura(Widget* widget) : widget_(widget) { > Style here is fine, leave it. > > https://codereview.chromium.org/213833018/diff/240001/ui/wm/public/tooltip_cl... > File ui/wm/public/tooltip_client.cc (right): > > https://codereview.chromium.org/213833018/diff/240001/ui/wm/public/tooltip_cl... > ui/wm/public/tooltip_client.cc:36: window->SetProperty(kTooltipIdKey, > static_cast<void*>(NULL)); > Is this really necessary? > MSVC complains without the cast, void aura::Window::SetProperty(const aura::WindowProperty<T> *,T)' : template parameter 'T' is ambiguous > https://codereview.chromium.org/213833018/diff/240001/ui/wm/public/tooltip_cl... > File ui/wm/public/tooltip_client.h (right): > > https://codereview.chromium.org/213833018/diff/240001/ui/wm/public/tooltip_cl... > ui/wm/public/tooltip_client.h:39: // Sets the |tooltip_text| and |id| pointers > as properties of the |window|. > This is rather confusing. How about: > Sets the text for the tooltip. The id is used to determine uniqueness when the > text does not change. For example, if the tooltip text does not change, but the > id does then the position of the tooltip is updated.
On Mon, Jun 2, 2014 at 10:48 PM, <mboc@opera.com> wrote: > On 2014/06/02 15:39:46, sky wrote: > > https://codereview.chromium.org/213833018/diff/240001/ui/views/corewm/tooltip... >> >> File ui/views/corewm/tooltip_controller.cc (right): > > > > https://codereview.chromium.org/213833018/diff/240001/ui/views/corewm/tooltip... >> >> ui/views/corewm/tooltip_controller.cc:290: if (tooltip_id) { >> Why the if here? Can't you always compare tooltip_id_ and tooltip_id? > > > > https://codereview.chromium.org/213833018/diff/240001/ui/views/widget/tooltip... >> >> File ui/views/widget/tooltip_manager_aura.cc (left): > > > > https://codereview.chromium.org/213833018/diff/240001/ui/views/widget/tooltip... >> >> ui/views/widget/tooltip_manager_aura.cc:29: >> TooltipManagerAura::TooltipManagerAura(Widget* widget) : widget_(widget) { >> Style here is fine, leave it. > > > > https://codereview.chromium.org/213833018/diff/240001/ui/wm/public/tooltip_cl... >> >> File ui/wm/public/tooltip_client.cc (right): > > > > https://codereview.chromium.org/213833018/diff/240001/ui/wm/public/tooltip_cl... >> >> ui/wm/public/tooltip_client.cc:36: window->SetProperty(kTooltipIdKey, >> static_cast<void*>(NULL)); >> Is this really necessary? I didn't mean the cast, I meant making SetTooltipText set both the text and the id. I think setting the id should be left to the caller. -Scott > > > > MSVC complains without the cast, > void aura::Window::SetProperty(const aura::WindowProperty<T> *,T)' : > template > parameter 'T' is ambiguous > > > > https://codereview.chromium.org/213833018/diff/240001/ui/wm/public/tooltip_cl... >> >> File ui/wm/public/tooltip_client.h (right): > > > > https://codereview.chromium.org/213833018/diff/240001/ui/wm/public/tooltip_cl... >> >> ui/wm/public/tooltip_client.h:39: // Sets the |tooltip_text| and |id| >> pointers >> as properties of the |window|. >> This is rather confusing. How about: >> Sets the text for the tooltip. The id is used to determine uniqueness when >> the >> text does not change. For example, if the tooltip text does not change, >> but > > the >> >> id does then the position of the tooltip is updated. > > > > https://codereview.chromium.org/213833018/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
You're right, done.
LGTM with the following change https://codereview.chromium.org/213833018/diff/280001/ui/wm/public/tooltip_cl... File ui/wm/public/tooltip_client.h (right): https://codereview.chromium.org/213833018/diff/280001/ui/wm/public/tooltip_cl... ui/wm/public/tooltip_client.h:44: AURA_EXPORT void UpdateTooltipId(Window* window, void* id); UpdateTooltipId->SetTooltipId
The CQ bit was checked by mboc@opera.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mboc@opera.com/213833018/300001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
On 2014/06/05 11:26:19, I haz the power (commit-bot) wrote: > Try jobs failed on following builders: > linux_chromium_chromeos_rel on tryserver.chromium > (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) Looks like something went wrong. I don't really have Linux builder, can someone take a look at this failed test on Lin? I will take a look at this as well but this issue may not be obvious.
The CQ bit was checked by mboc@opera.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mboc@opera.com/213833018/320001
On 2014/06/13 07:55:17, I haz the power (commit-bot) wrote: > CQ is trying da patch. Follow status at > https://chromium-status.appspot.com/cq/mboc@opera.com/213833018/320001 Looks like my fix resolved the problem.
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...) win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
Message was sent while issue was closed.
Change committed as 277010 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
