Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2645813002:
Add TaskRunnerHelper::get for Frame that casts to LocalFrame if possible or uses nullptr. (Closed)
|
Created:
3 years, 11 months ago by joelhockey Modified:
3 years, 10 months ago CC:
blink-reviews, blink-reviews-dom_chromium.org, chromium-reviews, dglazkov+blink, eae+blinkwatch, rwlbuis, sof Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd TaskRunnerHelper::get for Frame that casts to LocalFrame if possible or uses nullptr.
BUG=624694
Patch Set 1 #
Messages
Total messages: 20 (4 generated)
joelhockey@chromium.org changed reviewers: + sashab@chromium.org
LGTM :-)
The CQ bit was checked by joelhockey@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by joelhockey@chromium.org
LGTM but please check with haraken or dcheng before landing :) Thx
joelhockey@chromium.org changed reviewers: + dcheng@chromium.org, haraken@chromium.org
dcheng/haraken it is helpful to add this static get method for https://codereview.chromium.org/2648433003
On 2017/01/19 05:03:57, joelhockey wrote: > dcheng/haraken it is helpful to add this static get method for > https://codereview.chromium.org/2648433003 Are there places where this would be useful? It seems like we should already have a LocalFrame in the places where we'd want to use this.
On 2017/01/19 at 05:25:52, dcheng wrote: > On 2017/01/19 05:03:57, joelhockey wrote: > > dcheng/haraken it is helpful to add this static get method for > > https://codereview.chromium.org/2648433003 > > Are there places where this would be useful? It seems like we should already have a LocalFrame in the places where we'd want to use this. This is useful in the CL that I linked to (https://codereview.chromium.org/2648433003), specifically at https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/frame/Pla... where the available Page object has only a Frame, but not a LocalFrame. From what I read at https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/page/Page..., there is no guarantee that a Frame is always a LocalFrame.
On 2017/01/19 05:37:13, joelhockey wrote: > On 2017/01/19 at 05:25:52, dcheng wrote: > > On 2017/01/19 05:03:57, joelhockey wrote: > > > dcheng/haraken it is helpful to add this static get method for > > > https://codereview.chromium.org/2648433003 > > > > Are there places where this would be useful? It seems like we should already > have a LocalFrame in the places where we'd want to use this. > > This is useful in the CL that I linked to > (https://codereview.chromium.org/2648433003), specifically at > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/frame/Pla... > where the available Page object has only a Frame, but not a LocalFrame. From > what I read at > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/page/Page..., > there is no guarantee that a Frame is always a LocalFrame. Oops, sorry, I missed that part of the message, sorry. IMO, for things that are page-associated, we should not use a frame-specific task queue. Just trying to use the task queue of the main frame doesn't work once the frame isn't local.
On 2017/01/19 at 05:59:53, dcheng wrote: > On 2017/01/19 05:37:13, joelhockey wrote: > > On 2017/01/19 at 05:25:52, dcheng wrote: > > > On 2017/01/19 05:03:57, joelhockey wrote: > > > > dcheng/haraken it is helpful to add this static get method for > > > > https://codereview.chromium.org/2648433003 > > > > > > Are there places where this would be useful? It seems like we should already > > have a LocalFrame in the places where we'd want to use this. > > > > This is useful in the CL that I linked to > > (https://codereview.chromium.org/2648433003), specifically at > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/frame/Pla... > > where the available Page object has only a Frame, but not a LocalFrame. From > > what I read at > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/page/Page..., > > there is no guarantee that a Frame is always a LocalFrame. > > Oops, sorry, I missed that part of the message, sorry. > > IMO, for things that are page-associated, we should not use a frame-specific task queue. Just trying to use the task queue of the main frame doesn't work once the frame isn't local. So, should I leave this code to use the original Timer class?
On 2017/01/19 06:06:53, joelhockey wrote: > On 2017/01/19 at 05:59:53, dcheng wrote: > > On 2017/01/19 05:37:13, joelhockey wrote: > > > On 2017/01/19 at 05:25:52, dcheng wrote: > > > > On 2017/01/19 05:03:57, joelhockey wrote: > > > > > dcheng/haraken it is helpful to add this static get method for > > > > > https://codereview.chromium.org/2648433003 > > > > > > > > Are there places where this would be useful? It seems like we should > already > > > have a LocalFrame in the places where we'd want to use this. > > > > > > This is useful in the CL that I linked to > > > (https://codereview.chromium.org/2648433003), specifically at > > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/frame/Pla... > > > where the available Page object has only a Frame, but not a LocalFrame. > From > > > what I read at > > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/page/Page..., > > > there is no guarantee that a Frame is always a LocalFrame. > > > > Oops, sorry, I missed that part of the message, sorry. > > > > IMO, for things that are page-associated, we should not use a frame-specific > task queue. Just trying to use the task queue of the main frame doesn't work > once the frame isn't local. > > So, should I leave this code to use the original Timer class? I looked a little more closely, and it looks like all the things that construct a PlatformEventController originally start with a frame or document. So in this case, I think we should actually fix the plumbing in PlatformEventController. WDYT?
On 2017/01/19 at 06:10:13, dcheng wrote: > On 2017/01/19 06:06:53, joelhockey wrote: > > On 2017/01/19 at 05:59:53, dcheng wrote: > > > On 2017/01/19 05:37:13, joelhockey wrote: > > > > On 2017/01/19 at 05:25:52, dcheng wrote: > > > > > On 2017/01/19 05:03:57, joelhockey wrote: > > > > > > dcheng/haraken it is helpful to add this static get method for > > > > > > https://codereview.chromium.org/2648433003 > > > > > > > > > > Are there places where this would be useful? It seems like we should > > already > > > > have a LocalFrame in the places where we'd want to use this. > > > > > > > > This is useful in the CL that I linked to > > > > (https://codereview.chromium.org/2648433003), specifically at > > > > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/frame/Pla... > > > > where the available Page object has only a Frame, but not a LocalFrame. > > From > > > > what I read at > > > > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/page/Page..., > > > > there is no guarantee that a Frame is always a LocalFrame. > > > > > > Oops, sorry, I missed that part of the message, sorry. > > > > > > IMO, for things that are page-associated, we should not use a frame-specific > > task queue. Just trying to use the task queue of the main frame doesn't work > > once the frame isn't local. > > > > So, should I leave this code to use the original Timer class? > > I looked a little more closely, and it looks like all the things that construct a PlatformEventController originally start with a frame or document. So in this case, I think we should actually fix the plumbing in PlatformEventController. WDYT? I'm very new to chrome. This is about my 3rd CL, so I don't have a whole lot of thoughts ;-) But what you are saying makes sense. I'll have a look at all the callers and see if I can change them all.
On 2017/01/19 06:17:46, joelhockey wrote: > On 2017/01/19 at 06:10:13, dcheng wrote: > > On 2017/01/19 06:06:53, joelhockey wrote: > > > On 2017/01/19 at 05:59:53, dcheng wrote: > > > > On 2017/01/19 05:37:13, joelhockey wrote: > > > > > On 2017/01/19 at 05:25:52, dcheng wrote: > > > > > > On 2017/01/19 05:03:57, joelhockey wrote: > > > > > > > dcheng/haraken it is helpful to add this static get method for > > > > > > > https://codereview.chromium.org/2648433003 > > > > > > > > > > > > Are there places where this would be useful? It seems like we should > > > already > > > > > have a LocalFrame in the places where we'd want to use this. > > > > > > > > > > This is useful in the CL that I linked to > > > > > (https://codereview.chromium.org/2648433003), specifically at > > > > > > > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/frame/Pla... > > > > > where the available Page object has only a Frame, but not a LocalFrame. > > > From > > > > > what I read at > > > > > > > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/page/Page..., > > > > > there is no guarantee that a Frame is always a LocalFrame. > > > > > > > > Oops, sorry, I missed that part of the message, sorry. > > > > > > > > IMO, for things that are page-associated, we should not use a > frame-specific > > > task queue. Just trying to use the task queue of the main frame doesn't work > > > once the frame isn't local. > > > > > > So, should I leave this code to use the original Timer class? > > > > I looked a little more closely, and it looks like all the things that > construct a PlatformEventController originally start with a frame or document. > So in this case, I think we should actually fix the plumbing in > PlatformEventController. WDYT? > > I'm very new to chrome. This is about my 3rd CL, so I don't have a whole lot of > thoughts ;-) > But what you are saying makes sense. I'll have a look at all the callers and > see if I can change them all. OK, feel free to reach out with any questions. I'll be around for another hour or two.
On 2017/01/19 at 06:18:44, dcheng wrote: > On 2017/01/19 06:17:46, joelhockey wrote: > > On 2017/01/19 at 06:10:13, dcheng wrote: > > > On 2017/01/19 06:06:53, joelhockey wrote: > > > > On 2017/01/19 at 05:59:53, dcheng wrote: > > > > > On 2017/01/19 05:37:13, joelhockey wrote: > > > > > > On 2017/01/19 at 05:25:52, dcheng wrote: > > > > > > > On 2017/01/19 05:03:57, joelhockey wrote: > > > > > > > > dcheng/haraken it is helpful to add this static get method for > > > > > > > > https://codereview.chromium.org/2648433003 > > > > > > > > > > > > > > Are there places where this would be useful? It seems like we should > > > > already > > > > > > have a LocalFrame in the places where we'd want to use this. > > > > > > > > > > > > This is useful in the CL that I linked to > > > > > > (https://codereview.chromium.org/2648433003), specifically at > > > > > > > > > > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/frame/Pla... > > > > > > where the available Page object has only a Frame, but not a LocalFrame. > > > > From > > > > > > what I read at > > > > > > > > > > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/page/Page..., > > > > > > there is no guarantee that a Frame is always a LocalFrame. > > > > > > > > > > Oops, sorry, I missed that part of the message, sorry. > > > > > > > > > > IMO, for things that are page-associated, we should not use a > > frame-specific > > > > task queue. Just trying to use the task queue of the main frame doesn't work > > > > once the frame isn't local. > > > > > > > > So, should I leave this code to use the original Timer class? > > > > > > I looked a little more closely, and it looks like all the things that > > construct a PlatformEventController originally start with a frame or document. > > So in this case, I think we should actually fix the plumbing in > > PlatformEventController. WDYT? > > > > I'm very new to chrome. This is about my 3rd CL, so I don't have a whole lot of > > thoughts ;-) > > But what you are saying makes sense. I'll have a look at all the callers and > > see if I can change them all. > > OK, feel free to reach out with any questions. I'll be around for another hour or two. Thanks Daniel. I have created https://codereview.chromium.org/2645823002 and I will delete this change.
On 2017/01/19 at 09:42:49, joelhockey wrote: > On 2017/01/19 at 06:18:44, dcheng wrote: > > On 2017/01/19 06:17:46, joelhockey wrote: > > > On 2017/01/19 at 06:10:13, dcheng wrote: > > > > On 2017/01/19 06:06:53, joelhockey wrote: > > > > > On 2017/01/19 at 05:59:53, dcheng wrote: > > > > > > On 2017/01/19 05:37:13, joelhockey wrote: > > > > > > > On 2017/01/19 at 05:25:52, dcheng wrote: > > > > > > > > On 2017/01/19 05:03:57, joelhockey wrote: > > > > > > > > > dcheng/haraken it is helpful to add this static get method for > > > > > > > > > https://codereview.chromium.org/2648433003 > > > > > > > > > > > > > > > > Are there places where this would be useful? It seems like we should > > > > > already > > > > > > > have a LocalFrame in the places where we'd want to use this. > > > > > > > > > > > > > > This is useful in the CL that I linked to > > > > > > > (https://codereview.chromium.org/2648433003), specifically at > > > > > > > > > > > > > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/frame/Pla... > > > > > > > where the available Page object has only a Frame, but not a LocalFrame. > > > > > From > > > > > > > what I read at > > > > > > > > > > > > > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/page/Page..., > > > > > > > there is no guarantee that a Frame is always a LocalFrame. > > > > > > > > > > > > Oops, sorry, I missed that part of the message, sorry. > > > > > > > > > > > > IMO, for things that are page-associated, we should not use a > > > frame-specific > > > > > task queue. Just trying to use the task queue of the main frame doesn't work > > > > > once the frame isn't local. > > > > > > > > > > So, should I leave this code to use the original Timer class? > > > > > > > > I looked a little more closely, and it looks like all the things that > > > construct a PlatformEventController originally start with a frame or document. > > > So in this case, I think we should actually fix the plumbing in > > > PlatformEventController. WDYT? > > > > > > I'm very new to chrome. This is about my 3rd CL, so I don't have a whole lot of > > > thoughts ;-) > > > But what you are saying makes sense. I'll have a look at all the callers and > > > see if I can change them all. > > > > OK, feel free to reach out with any questions. I'll be around for another hour or two. > > Thanks Daniel. I have created https://codereview.chromium.org/2645823002 and I will delete this change. Daniel, could we got back to this approach?
On 2017/01/20 09:36:03, joelhockey wrote: > On 2017/01/19 at 09:42:49, joelhockey wrote: > > On 2017/01/19 at 06:18:44, dcheng wrote: > > > On 2017/01/19 06:17:46, joelhockey wrote: > > > > On 2017/01/19 at 06:10:13, dcheng wrote: > > > > > On 2017/01/19 06:06:53, joelhockey wrote: > > > > > > On 2017/01/19 at 05:59:53, dcheng wrote: > > > > > > > On 2017/01/19 05:37:13, joelhockey wrote: > > > > > > > > On 2017/01/19 at 05:25:52, dcheng wrote: > > > > > > > > > On 2017/01/19 05:03:57, joelhockey wrote: > > > > > > > > > > dcheng/haraken it is helpful to add this static get method for > > > > > > > > > > https://codereview.chromium.org/2648433003 > > > > > > > > > > > > > > > > > > Are there places where this would be useful? It seems like we > should > > > > > > already > > > > > > > > have a LocalFrame in the places where we'd want to use this. > > > > > > > > > > > > > > > > This is useful in the CL that I linked to > > > > > > > > (https://codereview.chromium.org/2648433003), specifically at > > > > > > > > > > > > > > > > > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/frame/Pla... > > > > > > > > where the available Page object has only a Frame, but not a > LocalFrame. > > > > > > From > > > > > > > > what I read at > > > > > > > > > > > > > > > > > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/page/Page..., > > > > > > > > there is no guarantee that a Frame is always a LocalFrame. > > > > > > > > > > > > > > Oops, sorry, I missed that part of the message, sorry. > > > > > > > > > > > > > > IMO, for things that are page-associated, we should not use a > > > > frame-specific > > > > > > task queue. Just trying to use the task queue of the main frame > doesn't work > > > > > > once the frame isn't local. > > > > > > > > > > > > So, should I leave this code to use the original Timer class? > > > > > > > > > > I looked a little more closely, and it looks like all the things that > > > > construct a PlatformEventController originally start with a frame or > document. > > > > So in this case, I think we should actually fix the plumbing in > > > > PlatformEventController. WDYT? > > > > > > > > I'm very new to chrome. This is about my 3rd CL, so I don't have a whole > lot of > > > > thoughts ;-) > > > > But what you are saying makes sense. I'll have a look at all the callers > and > > > > see if I can change them all. > > > > > > OK, feel free to reach out with any questions. I'll be around for another > hour or two. > > > > Thanks Daniel. I have created https://codereview.chromium.org/2645823002 and > I will delete this change. > > Daniel, could we got back to this approach? Doesn't that CL just need to do a null check for frame then in PlatformEventController? e.g. frame ? frame->page() : nullptr
On 2017/01/20 at 19:04:37, dcheng wrote: > On 2017/01/20 09:36:03, joelhockey wrote: > > On 2017/01/19 at 09:42:49, joelhockey wrote: > > > On 2017/01/19 at 06:18:44, dcheng wrote: > > > > On 2017/01/19 06:17:46, joelhockey wrote: > > > > > On 2017/01/19 at 06:10:13, dcheng wrote: > > > > > > On 2017/01/19 06:06:53, joelhockey wrote: > > > > > > > On 2017/01/19 at 05:59:53, dcheng wrote: > > > > > > > > On 2017/01/19 05:37:13, joelhockey wrote: > > > > > > > > > On 2017/01/19 at 05:25:52, dcheng wrote: > > > > > > > > > > On 2017/01/19 05:03:57, joelhockey wrote: > > > > > > > > > > > dcheng/haraken it is helpful to add this static get method for > > > > > > > > > > > https://codereview.chromium.org/2648433003 > > > > > > > > > > > > > > > > > > > > Are there places where this would be useful? It seems like we > > should > > > > > > > already > > > > > > > > > have a LocalFrame in the places where we'd want to use this. > > > > > > > > > > > > > > > > > > This is useful in the CL that I linked to > > > > > > > > > (https://codereview.chromium.org/2648433003), specifically at > > > > > > > > > > > > > > > > > > > > > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/frame/Pla... > > > > > > > > > where the available Page object has only a Frame, but not a > > LocalFrame. > > > > > > > From > > > > > > > > > what I read at > > > > > > > > > > > > > > > > > > > > > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/page/Page..., > > > > > > > > > there is no guarantee that a Frame is always a LocalFrame. > > > > > > > > > > > > > > > > Oops, sorry, I missed that part of the message, sorry. > > > > > > > > > > > > > > > > IMO, for things that are page-associated, we should not use a > > > > > frame-specific > > > > > > > task queue. Just trying to use the task queue of the main frame > > doesn't work > > > > > > > once the frame isn't local. > > > > > > > > > > > > > > So, should I leave this code to use the original Timer class? > > > > > > > > > > > > I looked a little more closely, and it looks like all the things that > > > > > construct a PlatformEventController originally start with a frame or > > document. > > > > > So in this case, I think we should actually fix the plumbing in > > > > > PlatformEventController. WDYT? > > > > > > > > > > I'm very new to chrome. This is about my 3rd CL, so I don't have a whole > > lot of > > > > > thoughts ;-) > > > > > But what you are saying makes sense. I'll have a look at all the callers > > and > > > > > see if I can change them all. > > > > > > > > OK, feel free to reach out with any questions. I'll be around for another > > hour or two. > > > > > > Thanks Daniel. I have created https://codereview.chromium.org/2645823002 and > > I will delete this change. > > > > Daniel, could we got back to this approach? > > Doesn't that CL just need to do a null check for frame then in PlatformEventController? e.g. frame ? frame->page() : nullptr Yes, that fixes things. Going back to https://codereview.chromium.org/2645823002, and I will close this change. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
