Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1466393002:
Support tooltip for HoverHighlightView (Closed)
DescriptionSupport tooltip for HoverHighlightView
BUG=
Committed: https://crrev.com/a00da4ed15a3bcd3d0ced74c917cc2c78452e701
Cr-Commit-Position: refs/heads/master@{#361308}
Patch Set 1 #
Total comments: 8
Patch Set 2 : #
Total comments: 2
Patch Set 3 : #
Messages
Total messages: 22 (8 generated)
fqj@chromium.org changed reviewers: + stevenjb@chromium.org
PTAL.
https://codereview.chromium.org/1466393002/diff/1/ash/system/tray/hover_highl... File ash/system/tray/hover_highlight_view.cc (right): https://codereview.chromium.org/1466393002/diff/1/ash/system/tray/hover_highl... ash/system/tray/hover_highlight_view.cc:250: } if (tooltip_.empty()) return false; tooltip = tooltip_; return true; https://codereview.chromium.org/1466393002/diff/1/ash/system/tray/hover_highl... ash/system/tray/hover_highlight_view.cc:255: } Trivial setters like this should be inlined in the header (and don't require a comment): void set_tooltip(...) { tooltip_ = tooltip; } https://codereview.chromium.org/1466393002/diff/1/ash/system/tray/hover_highl... File ash/system/tray/hover_highlight_view.h (right): https://codereview.chromium.org/1466393002/diff/1/ash/system/tray/hover_highl... ash/system/tray/hover_highlight_view.h:70: base::string16* tooltip) const override; Overrides should be in a separate commented section, typically before other functions. // views::View bool GetTooltipText... https://codereview.chromium.org/1466393002/diff/1/ash/system/tray/hover_highl... ash/system/tray/hover_highlight_view.h:71: void SetTooltipText(const base::string16& tooltip); New methods should have a comment.
PTAL https://codereview.chromium.org/1466393002/diff/1/ash/system/tray/hover_highl... File ash/system/tray/hover_highlight_view.cc (right): https://codereview.chromium.org/1466393002/diff/1/ash/system/tray/hover_highl... ash/system/tray/hover_highlight_view.cc:250: } On 2015/11/23 20:26:24, stevenjb wrote: > if (tooltip_.empty()) > return false; > tooltip = tooltip_; > return true; > Done. https://codereview.chromium.org/1466393002/diff/1/ash/system/tray/hover_highl... ash/system/tray/hover_highlight_view.cc:255: } On 2015/11/23 20:26:24, stevenjb wrote: > Trivial setters like this should be inlined in the header (and don't require a > comment): > > void set_tooltip(...) { tooltip_ = tooltip; } Done. https://codereview.chromium.org/1466393002/diff/1/ash/system/tray/hover_highl... File ash/system/tray/hover_highlight_view.h (right): https://codereview.chromium.org/1466393002/diff/1/ash/system/tray/hover_highl... ash/system/tray/hover_highlight_view.h:70: base::string16* tooltip) const override; On 2015/11/23 20:26:24, stevenjb wrote: > Overrides should be in a separate commented section, typically before other > functions. > > // views::View > bool GetTooltipText... Done. https://codereview.chromium.org/1466393002/diff/1/ash/system/tray/hover_highl... ash/system/tray/hover_highlight_view.h:71: void SetTooltipText(const base::string16& tooltip); On 2015/11/23 20:26:24, stevenjb wrote: > New methods should have a comment. Updated to a trivial setters in headers with implementations. And you said trivial setters don't require one.
https://codereview.chromium.org/1466393002/diff/20001/ash/system/tray/hover_h... File ash/system/tray/hover_highlight_view.cc (right): https://codereview.chromium.org/1466393002/diff/20001/ash/system/tray/hover_h... ash/system/tray/hover_highlight_view.cc:249: } Move this to match the header order. https://codereview.chromium.org/1466393002/diff/20001/ash/system/tray/hover_h... File ash/system/tray/hover_highlight_view.h (right): https://codereview.chromium.org/1466393002/diff/20001/ash/system/tray/hover_h... ash/system/tray/hover_highlight_view.h:73: void SetTooltipText(const base::string16& tooltip) { tooltip_ = tooltip; } set_tooltup() (Trivial setters use the naming convention set_foo() just as getters use the convention foo()).
Done PTAL.
lgtm
The CQ bit was checked by fqj@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1466393002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1466393002/40001
The CQ bit was unchecked by fqj@chromium.org
The CQ bit was checked by fqj@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1466393002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1466393002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by fqj@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1466393002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1466393002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by fqj@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1466393002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1466393002/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/a00da4ed15a3bcd3d0ced74c917cc2c78452e701 Cr-Commit-Position: refs/heads/master@{#361308} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
