Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1653213002:
Devtools: Use Event.deepPath as a function if available. (Closed)
|
Created:
4 years, 10 months ago by samli Modified:
4 years, 10 months ago 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, sergeyv+blink_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDevtools: Use Event.deepPath as a function if available.
In crrev.com/1637813002 Event.deepPath changes from an attribute to a
function. Event.deepPath is the prefered method for retriving all
elements at point in an event. This change also adds a fallback to
Event.path for old front-ends targeting Event.deepPath as an attribute.
Committed: https://crrev.com/9e13f37d07379cf402dfe7ad981923a8eb8787f0
Cr-Commit-Position: refs/heads/master@{#374084}
Patch Set 1 #
Total comments: 4
Patch Set 2 : #Patch Set 3 : #
Total comments: 3
Patch Set 4 : #
Messages
Total messages: 24 (6 generated)
samli@chromium.org changed reviewers: + pfeldman@chromium.org
parentElementOrShadowHost() isn't the same. Want to traverse all elements under mouse point.
https://codereview.chromium.org/1653213002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/ui/Tooltip.js (right): https://codereview.chromium.org/1653213002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/ui/Tooltip.js:36: var path = event.deepPath && typeof event.deepPath == "function" ? event.deepPath() : event.path; You just made it undefined, no point in checking for it anymore, right?
dgozman@chromium.org changed reviewers: + dgozman@chromium.org
https://codereview.chromium.org/1653213002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/devtools.js (right): https://codereview.chromium.org/1653213002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/devtools.js:966: if (Object.getOwnPropertyDescriptor(Event.prototype, "deepPath")) Please move this code (and a block above while we are here) into installBackwardsCompatibility method.
https://codereview.chromium.org/1653213002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/devtools.js (right): https://codereview.chromium.org/1653213002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/devtools.js:966: if (Object.getOwnPropertyDescriptor(Event.prototype, "deepPath")) On 2016/02/02 at 02:45:45, dgozman wrote: > Please move this code (and a block above while we are here) into installBackwardsCompatibility method. Done. https://codereview.chromium.org/1653213002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/ui/Tooltip.js (right): https://codereview.chromium.org/1653213002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/ui/Tooltip.js:36: var path = event.deepPath && typeof event.deepPath == "function" ? event.deepPath() : event.path; On 2016/02/01 at 22:52:17, pfeldman wrote: > You just made it undefined, no point in checking for it anymore, right? True, but I wanted to be able to remove the bit in devtools.js at a later point and use deepPath().
> True, but I wanted to be able to remove the bit in devtools.js at a later point > and use deepPath(). You will never be able to remove it in devtools.js (years). So you might want to conditionally erase it there then.
On 2016/02/03 at 00:09:09, pfeldman wrote: > > True, but I wanted to be able to remove the bit in devtools.js at a later point > > and use deepPath(). > > You will never be able to remove it in devtools.js (years). So you might want to conditionally erase it there then. Ok, done.
https://codereview.chromium.org/1653213002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/devtools.js (right): https://codereview.chromium.org/1653213002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/devtools.js:913: if (Object.getOwnPropertyDescriptor(Event.prototype, "deepPath")) Wouldn't it remove it at all times? I am not following this :)
https://codereview.chromium.org/1653213002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/devtools.js (right): https://codereview.chromium.org/1653213002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/devtools.js:913: if (Object.getOwnPropertyDescriptor(Event.prototype, "deepPath")) On 2016/02/03 at 00:16:24, pfeldman wrote: > Wouldn't it remove it at all times? I am not following this :) Yes. You want me to only remove it on attribute ones?
ping
https://codereview.chromium.org/1653213002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/devtools.js (right): https://codereview.chromium.org/1653213002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/devtools.js:913: if (Object.getOwnPropertyDescriptor(Event.prototype, "deepPath")) On 2016/02/03 00:17:57, samli wrote: > On 2016/02/03 at 00:16:24, pfeldman wrote: > > Wouldn't it remove it at all times? I am not following this :) > > Yes. You want me to only remove it on attribute ones? I was thinking of simply executing "Event.prototype.deepPath = undefined;" unconditionally.
On 2016/02/04 at 00:19:58, pfeldman wrote: > https://codereview.chromium.org/1653213002/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/devtools/front_end/devtools.js (right): > > https://codereview.chromium.org/1653213002/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/devtools/front_end/devtools.js:913: if (Object.getOwnPropertyDescriptor(Event.prototype, "deepPath")) > On 2016/02/03 00:17:57, samli wrote: > > On 2016/02/03 at 00:16:24, pfeldman wrote: > > > Wouldn't it remove it at all times? I am not following this :) > > > > Yes. You want me to only remove it on attribute ones? > > I was thinking of simply executing "Event.prototype.deepPath = undefined;" unconditionally. Ok, done.
The CQ bit was checked by pfeldman@chromium.org
lgtm
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1653213002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1653213002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by samli@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1653213002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1653213002/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Devtools: Use Event.deepPath as a function if available. In crrev.com/1637813002 Event.deepPath changes from an attribute to a function. Event.deepPath is the prefered method for retriving all elements at point in an event. This change also adds a fallback to Event.path for old front-ends targeting Event.deepPath as an attribute. ========== to ========== Devtools: Use Event.deepPath as a function if available. In crrev.com/1637813002 Event.deepPath changes from an attribute to a function. Event.deepPath is the prefered method for retriving all elements at point in an event. This change also adds a fallback to Event.path for old front-ends targeting Event.deepPath as an attribute. Committed: https://crrev.com/9e13f37d07379cf402dfe7ad981923a8eb8787f0 Cr-Commit-Position: refs/heads/master@{#374084} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/9e13f37d07379cf402dfe7ad981923a8eb8787f0 Cr-Commit-Position: refs/heads/master@{#374084} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
