Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2551283002:
[DevTools] Fix null tooltip errors (Closed)
|
Created:
4 years ago by phulce Modified:
4 years ago Reviewers:
lushnikov CC:
apavlov+blink_chromium.org, blink-reviews, caseq+blink_chromium.org, chromium-reviews, devtools-reviews_chromium.org, kozyatinskiy+blink_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, pfeldman Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[DevTools] Fix null tooltip errors
Discovered during work on 166637.
DataGrid (and potentially other consumers) attempt to set a null or undefined
'title' attribute on a DOM node which eventually triggers Tooltip.install.
Since this essentially equates to unsetting the title, the Tooltip should
properly handle such input and remove the tooltip symbol.
BUG=none
R=lushnikov
Committed: https://crrev.com/aa128ae9b0f65904743b0c6fa15919fbd1efb268
Cr-Commit-Position: refs/heads/master@{#436850}
Patch Set 1 #
Total comments: 6
Patch Set 2 : address feedback #Messages
Total messages: 22 (11 generated)
Description was changed from ========== [DevTools] Fix null tooltip errors Discovered during work on 166637. DataGrid (and potentially other consumers) attempt to set a null or undefined 'title' attribute on a DOM node which eventually triggers Tooltip.install. Since this essentially equates to unsetting the title, the Tooltip should properly handle such input and remove the tooltip symbol. BUG=none ========== to ========== [DevTools] Fix null tooltip errors Discovered during work on 166637. DataGrid (and potentially other consumers) attempt to set a null or undefined 'title' attribute on a DOM node which eventually triggers Tooltip.install. Since this essentially equates to unsetting the title, the Tooltip should properly handle such input and remove the tooltip symbol. BUG=none ==========
phulce@chromium.org changed reviewers: + lushnikov@chromium.org
lgtm, thanks! could you please wrap the issue description in a multiple lines? https://codereview.chromium.org/2551283002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/ui/Tooltip.js (right): https://codereview.chromium.org/2551283002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/ui/Tooltip.js:32: * Installs the tooltipContent to be shown on mouseover, called when title is set on an Element. style: please, drop the documentation comment (i know, that sounds shocking, but we don't use any) https://codereview.chromium.org/2551283002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/ui/Tooltip.js:41: if (tooltipContent === null || tooltipContent === '') { if (!tooltipContent) { ... }
Description was changed from ========== [DevTools] Fix null tooltip errors Discovered during work on 166637. DataGrid (and potentially other consumers) attempt to set a null or undefined 'title' attribute on a DOM node which eventually triggers Tooltip.install. Since this essentially equates to unsetting the title, the Tooltip should properly handle such input and remove the tooltip symbol. BUG=none ========== to ========== [DevTools] Fix null tooltip errors Discovered during work on 166637. DataGrid (and potentially other consumers) attempt to set a null or undefined 'title' attribute on a DOM node which eventually triggers Tooltip.install. Since this essentially equates to unsetting the title, the Tooltip should properly handle such input and remove the tooltip symbol. BUG=none R=lushnikov ==========
Description was changed from ========== [DevTools] Fix null tooltip errors Discovered during work on 166637. DataGrid (and potentially other consumers) attempt to set a null or undefined 'title' attribute on a DOM node which eventually triggers Tooltip.install. Since this essentially equates to unsetting the title, the Tooltip should properly handle such input and remove the tooltip symbol. BUG=none R=lushnikov ========== to ========== [DevTools] Fix null tooltip errors Discovered during work on 166637. DataGrid (and potentially other consumers) attempt to set a null or undefined 'title' attribute on a DOM node which eventually triggers Tooltip.install. Since this essentially equates to unsetting the title, the Tooltip should properly handle such input and remove the tooltip symbol. BUG=none R=lushnikov ==========
https://codereview.chromium.org/2551283002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/ui/Tooltip.js (right): https://codereview.chromium.org/2551283002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/ui/Tooltip.js:32: * Installs the tooltipContent to be shown on mouseover, called when title is set on an Element. On 2016/12/06 at 01:19:37, lushnikov wrote: > style: please, drop the documentation comment (i know, that sounds shocking, but we don't use any) Isn't that old policy? Blink is open to comments for documentation now, aren't we?
https://codereview.chromium.org/2551283002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/ui/Tooltip.js (right): https://codereview.chromium.org/2551283002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/ui/Tooltip.js:32: * Installs the tooltipContent to be shown on mouseover, called when title is set on an Element. Let's discuss this on Thursday! We should either write comments consistently or disregard them altogether. I'd rather not, since front-end is not *that* big and moves quite fast.
https://codereview.chromium.org/2551283002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/ui/Tooltip.js (right): https://codereview.chromium.org/2551283002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/ui/Tooltip.js:32: * Installs the tooltipContent to be shown on mouseover, called when title is set on an Element. On 2016/12/06 01:19:37, lushnikov wrote: > style: please, drop the documentation comment (i know, that sounds shocking, but > we don't use any) Done. https://codereview.chromium.org/2551283002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/ui/Tooltip.js:41: if (tooltipContent === null || tooltipContent === '') { On 2016/12/06 01:19:36, lushnikov wrote: > if (!tooltipContent) { > ... > } Done.
The CQ bit was checked by phulce@chromium.org to run a CQ dry run
The CQ bit was unchecked by phulce@chromium.org
The CQ bit was checked by phulce@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lushnikov@chromium.org Link to the patchset: https://codereview.chromium.org/2551283002/#ps20001 (title: "address feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1481070656414190,
"parent_rev": "cb440fb335f61d15bba651cfa599a4bf5db994cc", "commit_rev":
"a8e8fc5451453cdc6076acf0775554f8ca200fb9"}
Message was sent while issue was closed.
Description was changed from ========== [DevTools] Fix null tooltip errors Discovered during work on 166637. DataGrid (and potentially other consumers) attempt to set a null or undefined 'title' attribute on a DOM node which eventually triggers Tooltip.install. Since this essentially equates to unsetting the title, the Tooltip should properly handle such input and remove the tooltip symbol. BUG=none R=lushnikov ========== to ========== [DevTools] Fix null tooltip errors Discovered during work on 166637. DataGrid (and potentially other consumers) attempt to set a null or undefined 'title' attribute on a DOM node which eventually triggers Tooltip.install. Since this essentially equates to unsetting the title, the Tooltip should properly handle such input and remove the tooltip symbol. BUG=none R=lushnikov ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== [DevTools] Fix null tooltip errors Discovered during work on 166637. DataGrid (and potentially other consumers) attempt to set a null or undefined 'title' attribute on a DOM node which eventually triggers Tooltip.install. Since this essentially equates to unsetting the title, the Tooltip should properly handle such input and remove the tooltip symbol. BUG=none R=lushnikov ========== to ========== [DevTools] Fix null tooltip errors Discovered during work on 166637. DataGrid (and potentially other consumers) attempt to set a null or undefined 'title' attribute on a DOM node which eventually triggers Tooltip.install. Since this essentially equates to unsetting the title, the Tooltip should properly handle such input and remove the tooltip symbol. BUG=none R=lushnikov Committed: https://crrev.com/aa128ae9b0f65904743b0c6fa15919fbd1efb268 Cr-Commit-Position: refs/heads/master@{#436850} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/aa128ae9b0f65904743b0c6fa15919fbd1efb268 Cr-Commit-Position: refs/heads/master@{#436850}
Message was sent while issue was closed.
On 2016/12/07 at 03:39:09, commit-bot wrote: > Patchset 2 (id:??) landed as https://crrev.com/aa128ae9b0f65904743b0c6fa15919fbd1efb268 > Cr-Commit-Position: refs/heads/master@{#436850} I've seen that issue while working on 166637, but haven't thought of submitting a fix as a separate CL (my bad!). I've solved it a bit differently than shown here though. In DataGrid.createCell cell value is being assigned to a node's title causing tooltip to intialize. There is no need for initializing the tooltip for empty cells, which have value of null, so I've replaced `else` with `else if(data !== null)` in DataGrid.createCell preventing said initialization. Solution shown here is probably better because it's general (even though it does have a small edgecase where 0 can't be set as a node's title anymore), but IMO it might still be a good idea to include the `data` check in DataGrid.createCell just to prevent unnecessary Tooltip.install calls. Sorry for blabbing so much about such a small issue, I clearly overthought it :)
Message was sent while issue was closed.
Hey Konrad! That sounds reasonable! Would you like to submit another CL?
Message was sent while issue was closed.
On 2016/12/08 at 21:36:16, lushnikov wrote: > Hey Konrad! > > That sounds reasonable! Would you like to submit another CL? I just did! https://codereview.chromium.org/2566463003 |
