Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
491483006:
Add accessibilityEnabled and inlineTextBoxAccessiblityEnabled to Settings, but don't use them yet. (Closed)
|
Created:
6 years, 4 months ago by aboxhall Modified:
6 years, 3 months ago CC:
abarth-chromium, blink-reviews, dglazkov+blink, jamesr Base URL:
svn://svn.chromium.org/blink/trunk Project:
blink Visibility:
Public. |
DescriptionAdd accessibilityEnabled and inlineTextBoxAccessiblityEnabled to Settings, but don't use them yet.
BUG=406622
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=181001
Patch Set 1 #
Total comments: 5
Patch Set 2 : Review comments #
Total comments: 2
Patch Set 3 : No setAccessibility methods on WebFrame #Patch Set 4 : Rebase #
Messages
Total messages: 23 (0 generated)
lgtm https://codereview.chromium.org/491483006/diff/1/Source/web/WebLocalFrameImpl... File Source/web/WebLocalFrameImpl.cpp (right): https://codereview.chromium.org/491483006/diff/1/Source/web/WebLocalFrameImpl... Source/web/WebLocalFrameImpl.cpp:579: fprintf(stderr, "setting inline text box accessibility to %d\n", enabled); don't forget to delete this https://codereview.chromium.org/491483006/diff/1/Source/web/WebRemoteFrameImpl.h File Source/web/WebRemoteFrameImpl.h (right): https://codereview.chromium.org/491483006/diff/1/Source/web/WebRemoteFrameImp... Source/web/WebRemoteFrameImpl.h:39: virtual void setAccessibilityEnabled(bool) OVERRIDE; I don't think it makes sense to do this on a remote frame. The topmost frame will never be remote. A remote frame represents a frame in the local frame tree that's running in another process; it doesn't have any nodes or renderers, this is just a way for local frames to post messages to it.
https://codereview.chromium.org/491483006/diff/1/Source/web/WebLocalFrameImpl... File Source/web/WebLocalFrameImpl.cpp (right): https://codereview.chromium.org/491483006/diff/1/Source/web/WebLocalFrameImpl... Source/web/WebLocalFrameImpl.cpp:579: fprintf(stderr, "setting inline text box accessibility to %d\n", enabled); On 2014/08/22 23:13:26, dmazzoni wrote: > don't forget to delete this Thanks :) https://codereview.chromium.org/491483006/diff/1/Source/web/WebRemoteFrameImpl.h File Source/web/WebRemoteFrameImpl.h (right): https://codereview.chromium.org/491483006/diff/1/Source/web/WebRemoteFrameImp... Source/web/WebRemoteFrameImpl.h:39: virtual void setAccessibilityEnabled(bool) OVERRIDE; On 2014/08/22 23:13:26, dmazzoni wrote: > I don't think it makes sense to do this on a remote frame. > > The topmost frame will never be remote. A remote frame represents a frame in the > local frame tree that's running in another process; it doesn't have any nodes or > renderers, this is just a way for local frames to post messages to it. I'll still need to implement it, but I can make it a noop (as many other things in the implementation are).
still lgtm; I'm happy with the ASSERT_NOT_REACHED. If we happen to trip this when --site-per-process gets flipped it will be worth debugging and figuring out the right solution.
https://codereview.chromium.org/491483006/diff/1/Source/web/WebRemoteFrameImpl.h File Source/web/WebRemoteFrameImpl.h (right): https://codereview.chromium.org/491483006/diff/1/Source/web/WebRemoteFrameImp... Source/web/WebRemoteFrameImpl.h:39: virtual void setAccessibilityEnabled(bool) OVERRIDE; On 2014/08/22 23:30:51, aboxhall wrote: > On 2014/08/22 23:13:26, dmazzoni wrote: > > I don't think it makes sense to do this on a remote frame. > > > > The topmost frame will never be remote. A remote frame represents a frame in > the > > local frame tree that's running in another process; it doesn't have any nodes > or > > renderers, this is just a way for local frames to post messages to it. > > I'll still need to implement it, but I can make it a noop (as many other things > in the implementation are). If this doesn't make sense to implement on remote frames, should the interface method be moved to WebLocalFrame.h instead?
On 2014/08/22 23:45:52, dcheng (OOO) wrote: > https://codereview.chromium.org/491483006/diff/1/Source/web/WebRemoteFrameImpl.h > File Source/web/WebRemoteFrameImpl.h (right): > > https://codereview.chromium.org/491483006/diff/1/Source/web/WebRemoteFrameImp... > Source/web/WebRemoteFrameImpl.h:39: virtual void setAccessibilityEnabled(bool) > OVERRIDE; > On 2014/08/22 23:30:51, aboxhall wrote: > > On 2014/08/22 23:13:26, dmazzoni wrote: > > > I don't think it makes sense to do this on a remote frame. > > > > > > The topmost frame will never be remote. A remote frame represents a frame in > > the > > > local frame tree that's running in another process; it doesn't have any > nodes > > or > > > renderers, this is just a way for local frames to post messages to it. > > > > I'll still need to implement it, but I can make it a noop (as many other > things > > in the implementation are). > > If this doesn't make sense to implement on remote frames, should the interface > method be moved to WebLocalFrame.h instead? Yeah, you could use isWebLocalFrame to check it a WebFrame is a WebLocalFrame.
On 2014/08/22 23:49:43, dmazzoni wrote: > On 2014/08/22 23:45:52, dcheng (OOO) wrote: > > > https://codereview.chromium.org/491483006/diff/1/Source/web/WebRemoteFrameImpl.h > > File Source/web/WebRemoteFrameImpl.h (right): > > > > > https://codereview.chromium.org/491483006/diff/1/Source/web/WebRemoteFrameImp... > > Source/web/WebRemoteFrameImpl.h:39: virtual void setAccessibilityEnabled(bool) > > OVERRIDE; > > On 2014/08/22 23:30:51, aboxhall wrote: > > > On 2014/08/22 23:13:26, dmazzoni wrote: > > > > I don't think it makes sense to do this on a remote frame. > > > > > > > > The topmost frame will never be remote. A remote frame represents a frame > in > > > the > > > > local frame tree that's running in another process; it doesn't have any > > nodes > > > or > > > > renderers, this is just a way for local frames to post messages to it. > > > > > > I'll still need to implement it, but I can make it a noop (as many other > > things > > > in the implementation are). > > > > If this doesn't make sense to implement on remote frames, should the interface > > method be moved to WebLocalFrame.h instead? > > Yeah, you could use isWebLocalFrame to check it a WebFrame is a WebLocalFrame. Why would this be different to the many other methods on WebRemoteFrame which use ASSERT_NOT_REACHED?
On 2014/08/22 at 23:49:43, dmazzoni wrote: > On 2014/08/22 23:45:52, dcheng (OOO) wrote: > > https://codereview.chromium.org/491483006/diff/1/Source/web/WebRemoteFrameImpl.h > > File Source/web/WebRemoteFrameImpl.h (right): > > > > https://codereview.chromium.org/491483006/diff/1/Source/web/WebRemoteFrameImp... > > Source/web/WebRemoteFrameImpl.h:39: virtual void setAccessibilityEnabled(bool) > > OVERRIDE; > > On 2014/08/22 23:30:51, aboxhall wrote: > > > On 2014/08/22 23:13:26, dmazzoni wrote: > > > > I don't think it makes sense to do this on a remote frame. > > > > > > > > The topmost frame will never be remote. A remote frame represents a frame in > > > the > > > > local frame tree that's running in another process; it doesn't have any > > nodes > > > or > > > > renderers, this is just a way for local frames to post messages to it. > > > > > > I'll still need to implement it, but I can make it a noop (as many other > > things > > > in the implementation are). > > > > If this doesn't make sense to implement on remote frames, should the interface > > method be moved to WebLocalFrame.h instead? > > Yeah, you could use isWebLocalFrame to check it a WebFrame is a WebLocalFrame. Sorry I think I replied too fast. Reading over the diff, it looks like the intent is to provide a toggle for these settings. So I have a few other comments instead: 1) "The topmost frame will never be remote.": this is not true. Since the frame tree is mirrored, a child frame in another process would have a remote main frame. 2) If --site-per-process flag is active, you'd want to toggle this setting for all renderers participating in rendering that page right? You can actually implement this in WebFrame.cpp then: RemoteFrames still have a FrameHost.
On 2014/08/22 at 23:53:55, aboxhall wrote: > On 2014/08/22 23:49:43, dmazzoni wrote: > > On 2014/08/22 23:45:52, dcheng (OOO) wrote: > > > > > https://codereview.chromium.org/491483006/diff/1/Source/web/WebRemoteFrameImpl.h > > > File Source/web/WebRemoteFrameImpl.h (right): > > > > > > > > https://codereview.chromium.org/491483006/diff/1/Source/web/WebRemoteFrameImp... > > > Source/web/WebRemoteFrameImpl.h:39: virtual void setAccessibilityEnabled(bool) > > > OVERRIDE; > > > On 2014/08/22 23:30:51, aboxhall wrote: > > > > On 2014/08/22 23:13:26, dmazzoni wrote: > > > > > I don't think it makes sense to do this on a remote frame. > > > > > > > > > > The topmost frame will never be remote. A remote frame represents a frame > > in > > > > the > > > > > local frame tree that's running in another process; it doesn't have any > > > nodes > > > > or > > > > > renderers, this is just a way for local frames to post messages to it. > > > > > > > > I'll still need to implement it, but I can make it a noop (as many other > > > things > > > > in the implementation are). > > > > > > If this doesn't make sense to implement on remote frames, should the interface > > > method be moved to WebLocalFrame.h instead? > > > > Yeah, you could use isWebLocalFrame to check it a WebFrame is a WebLocalFrame. > > Why would this be different to the many other methods on WebRemoteFrame which use ASSERT_NOT_REACHED? When I renamed WebFrame to WebLocalFrame and then created WebRemoteFrame, I stubbed out all the WebFrame methods on WebRemoteFrame, with the long-term plan of removing those stubs.
On 2014/08/22 23:56:26, dcheng (OOO) wrote: > On 2014/08/22 at 23:49:43, dmazzoni wrote: > > On 2014/08/22 23:45:52, dcheng (OOO) wrote: > > > > https://codereview.chromium.org/491483006/diff/1/Source/web/WebRemoteFrameImpl.h > > > File Source/web/WebRemoteFrameImpl.h (right): > > > > > > > https://codereview.chromium.org/491483006/diff/1/Source/web/WebRemoteFrameImp... > > > Source/web/WebRemoteFrameImpl.h:39: virtual void > setAccessibilityEnabled(bool) > > > OVERRIDE; > > > On 2014/08/22 23:30:51, aboxhall wrote: > > > > On 2014/08/22 23:13:26, dmazzoni wrote: > > > > > I don't think it makes sense to do this on a remote frame. > > > > > > > > > > The topmost frame will never be remote. A remote frame represents a > frame in > > > > the > > > > > local frame tree that's running in another process; it doesn't have any > > > nodes > > > > or > > > > > renderers, this is just a way for local frames to post messages to it. > > > > > > > > I'll still need to implement it, but I can make it a noop (as many other > > > things > > > > in the implementation are). > > > > > > If this doesn't make sense to implement on remote frames, should the > interface > > > method be moved to WebLocalFrame.h instead? > > > > Yeah, you could use isWebLocalFrame to check it a WebFrame is a WebLocalFrame. > > Sorry I think I replied too fast. Reading over the diff, it looks like the > intent is to provide a toggle for these settings. So I have a few other comments > instead: > > 1) "The topmost frame will never be remote.": this is not true. Since the frame > tree is mirrored, a child frame in another process would have a remote main > frame. > > 2) If --site-per-process flag is active, you'd want to toggle this setting for > all renderers participating in rendering that page right? You can actually > implement this in WebFrame.cpp then: RemoteFrames still have a FrameHost. WebFrame doesn't seem to know about the FrameHost, though. How would I access it?
On 2014/08/22 23:57:14, dcheng (OOO) wrote: > On 2014/08/22 at 23:53:55, aboxhall wrote: > > On 2014/08/22 23:49:43, dmazzoni wrote: > > > On 2014/08/22 23:45:52, dcheng (OOO) wrote: > > > > > > > > https://codereview.chromium.org/491483006/diff/1/Source/web/WebRemoteFrameImpl.h > > > > File Source/web/WebRemoteFrameImpl.h (right): > > > > > > > > > > > > https://codereview.chromium.org/491483006/diff/1/Source/web/WebRemoteFrameImp... > > > > Source/web/WebRemoteFrameImpl.h:39: virtual void > setAccessibilityEnabled(bool) > > > > OVERRIDE; > > > > On 2014/08/22 23:30:51, aboxhall wrote: > > > > > On 2014/08/22 23:13:26, dmazzoni wrote: > > > > > > I don't think it makes sense to do this on a remote frame. > > > > > > > > > > > > The topmost frame will never be remote. A remote frame represents a > frame > > > in > > > > > the > > > > > > local frame tree that's running in another process; it doesn't have > any > > > > nodes > > > > > or > > > > > > renderers, this is just a way for local frames to post messages to it. > > > > > > > > > > I'll still need to implement it, but I can make it a noop (as many other > > > > things > > > > > in the implementation are). > > > > > > > > If this doesn't make sense to implement on remote frames, should the > interface > > > > method be moved to WebLocalFrame.h instead? > > > > > > Yeah, you could use isWebLocalFrame to check it a WebFrame is a > WebLocalFrame. > > > > Why would this be different to the many other methods on WebRemoteFrame which > use ASSERT_NOT_REACHED? > > When I renamed WebFrame to WebLocalFrame and then created WebRemoteFrame, I > stubbed out all the WebFrame methods on WebRemoteFrame, with the long-term plan > of removing those stubs. Ah, got it, thanks.
On 2014/08/23 at 00:02:43, aboxhall wrote: > On 2014/08/22 23:56:26, dcheng (OOO) wrote: > > On 2014/08/22 at 23:49:43, dmazzoni wrote: > > > On 2014/08/22 23:45:52, dcheng (OOO) wrote: > > > > > > https://codereview.chromium.org/491483006/diff/1/Source/web/WebRemoteFrameImpl.h > > > > File Source/web/WebRemoteFrameImpl.h (right): > > > > > > > > > > https://codereview.chromium.org/491483006/diff/1/Source/web/WebRemoteFrameImp... > > > > Source/web/WebRemoteFrameImpl.h:39: virtual void > > setAccessibilityEnabled(bool) > > > > OVERRIDE; > > > > On 2014/08/22 23:30:51, aboxhall wrote: > > > > > On 2014/08/22 23:13:26, dmazzoni wrote: > > > > > > I don't think it makes sense to do this on a remote frame. > > > > > > > > > > > > The topmost frame will never be remote. A remote frame represents a > > frame in > > > > > the > > > > > > local frame tree that's running in another process; it doesn't have any > > > > nodes > > > > > or > > > > > > renderers, this is just a way for local frames to post messages to it. > > > > > > > > > > I'll still need to implement it, but I can make it a noop (as many other > > > > things > > > > > in the implementation are). > > > > > > > > If this doesn't make sense to implement on remote frames, should the > > interface > > > > method be moved to WebLocalFrame.h instead? > > > > > > Yeah, you could use isWebLocalFrame to check it a WebFrame is a WebLocalFrame. > > > > Sorry I think I replied too fast. Reading over the diff, it looks like the > > intent is to provide a toggle for these settings. So I have a few other comments > > instead: > > > > 1) "The topmost frame will never be remote.": this is not true. Since the frame > > tree is mirrored, a child frame in another process would have a remote main > > frame. > > > > 2) If --site-per-process flag is active, you'd want to toggle this setting for > > all renderers participating in rendering that page right? You can actually > > implement this in WebFrame.cpp then: RemoteFrames still have a FrameHost. > > WebFrame doesn't seem to know about the FrameHost, though. How would I access it? You have to cheat a little--there's a toCoreFrame() helper which turns arbitrary WebFrames into Frames.
On 2014/08/23 00:11:32, dcheng (OOO) wrote: > On 2014/08/23 at 00:02:43, aboxhall wrote: > > On 2014/08/22 23:56:26, dcheng (OOO) wrote: > > > On 2014/08/22 at 23:49:43, dmazzoni wrote: > > > > On 2014/08/22 23:45:52, dcheng (OOO) wrote: > > > > > > > > > https://codereview.chromium.org/491483006/diff/1/Source/web/WebRemoteFrameImpl.h > > > > > File Source/web/WebRemoteFrameImpl.h (right): > > > > > > > > > > > > > > https://codereview.chromium.org/491483006/diff/1/Source/web/WebRemoteFrameImp... > > > > > Source/web/WebRemoteFrameImpl.h:39: virtual void > > > setAccessibilityEnabled(bool) > > > > > OVERRIDE; > > > > > On 2014/08/22 23:30:51, aboxhall wrote: > > > > > > On 2014/08/22 23:13:26, dmazzoni wrote: > > > > > > > I don't think it makes sense to do this on a remote frame. > > > > > > > > > > > > > > The topmost frame will never be remote. A remote frame represents a > > > frame in > > > > > > the > > > > > > > local frame tree that's running in another process; it doesn't have > any > > > > > nodes > > > > > > or > > > > > > > renderers, this is just a way for local frames to post messages to > it. > > > > > > > > > > > > I'll still need to implement it, but I can make it a noop (as many > other > > > > > things > > > > > > in the implementation are). > > > > > > > > > > If this doesn't make sense to implement on remote frames, should the > > > interface > > > > > method be moved to WebLocalFrame.h instead? > > > > > > > > Yeah, you could use isWebLocalFrame to check it a WebFrame is a > WebLocalFrame. > > > > > > Sorry I think I replied too fast. Reading over the diff, it looks like the > > > intent is to provide a toggle for these settings. So I have a few other > comments > > > instead: > > > > > > 1) "The topmost frame will never be remote.": this is not true. Since the > frame > > > tree is mirrored, a child frame in another process would have a remote main > > > frame. > > > > > > 2) If --site-per-process flag is active, you'd want to toggle this setting > for > > > all renderers participating in rendering that page right? You can actually > > > implement this in WebFrame.cpp then: RemoteFrames still have a FrameHost. > > > > WebFrame doesn't seem to know about the FrameHost, though. How would I access > it? > > You have to cheat a little--there's a toCoreFrame() helper which turns arbitrary > WebFrames into Frames. Only if BLINK_IMPLEMENTATION is true, apparently, so I'd need to check that before I call it presumably. Any advice?
On 2014/08/23 at 00:31:47, aboxhall wrote: > On 2014/08/23 00:11:32, dcheng (OOO) wrote: > > On 2014/08/23 at 00:02:43, aboxhall wrote: > > > On 2014/08/22 23:56:26, dcheng (OOO) wrote: > > > > On 2014/08/22 at 23:49:43, dmazzoni wrote: > > > > > On 2014/08/22 23:45:52, dcheng (OOO) wrote: > > > > > > > > > > > > https://codereview.chromium.org/491483006/diff/1/Source/web/WebRemoteFrameImpl.h > > > > > > File Source/web/WebRemoteFrameImpl.h (right): > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/491483006/diff/1/Source/web/WebRemoteFrameImp... > > > > > > Source/web/WebRemoteFrameImpl.h:39: virtual void > > > > setAccessibilityEnabled(bool) > > > > > > OVERRIDE; > > > > > > On 2014/08/22 23:30:51, aboxhall wrote: > > > > > > > On 2014/08/22 23:13:26, dmazzoni wrote: > > > > > > > > I don't think it makes sense to do this on a remote frame. > > > > > > > > > > > > > > > > The topmost frame will never be remote. A remote frame represents a > > > > frame in > > > > > > > the > > > > > > > > local frame tree that's running in another process; it doesn't have > > any > > > > > > nodes > > > > > > > or > > > > > > > > renderers, this is just a way for local frames to post messages to > > it. > > > > > > > > > > > > > > I'll still need to implement it, but I can make it a noop (as many > > other > > > > > > things > > > > > > > in the implementation are). > > > > > > > > > > > > If this doesn't make sense to implement on remote frames, should the > > > > interface > > > > > > method be moved to WebLocalFrame.h instead? > > > > > > > > > > Yeah, you could use isWebLocalFrame to check it a WebFrame is a > > WebLocalFrame. > > > > > > > > Sorry I think I replied too fast. Reading over the diff, it looks like the > > > > intent is to provide a toggle for these settings. So I have a few other > > comments > > > > instead: > > > > > > > > 1) "The topmost frame will never be remote.": this is not true. Since the > > frame > > > > tree is mirrored, a child frame in another process would have a remote main > > > > frame. > > > > > > > > 2) If --site-per-process flag is active, you'd want to toggle this setting > > for > > > > all renderers participating in rendering that page right? You can actually > > > > implement this in WebFrame.cpp then: RemoteFrames still have a FrameHost. > > > > > > WebFrame doesn't seem to know about the FrameHost, though. How would I access > > it? > > > > You have to cheat a little--there's a toCoreFrame() helper which turns arbitrary > > WebFrames into Frames. > > Only if BLINK_IMPLEMENTATION is true, apparently, so I'd need to check that before I call it presumably. Any advice? BLINK_IMPLEMENTATION is a trick to make implementation details of the Source/web layer only visible from files actually in the Blink repository. BLINK_IMPLEMENTATION will always be defined for a .cc file in third_party/WebKit, so there's no issue with using it freely here. For example, if you poke around, you'll see that WebString has conversion operators to/from WTF::String--but because they are wrapped in a BLINK_IMPLEMENTATION guard, code outside third_party/WebKit wouldn't be able to depend on those conversions.
On 2014/08/22 23:56:26, dcheng (OOO) wrote: > 1) "The topmost frame will never be remote.": this is not true. Since the frame > tree is mirrored, a child frame in another process would have a remote main > frame. You're right. More accurately, I should have said that the frame that Chrome calls this function on should never be remote. Even with --site-per-process, we trigger enabling accessibility from the browser side and only for real RenderFrameHosts, not proxies - so if we did call it on a remote frame that'd be a bug. > 2) If --site-per-process flag is active, you'd want to toggle this setting for > all renderers participating in rendering that page right? You can actually > implement this in WebFrame.cpp then: RemoteFrames still have a FrameHost. This is triggered from the browser side, so we should just do the one frame.
You shouldn't need to touch any of the Web*Frame* classes. WebSettings should be sufficient. https://codereview.chromium.org/491483006/diff/20001/public/web/WebFrame.h File public/web/WebFrame.h (right): https://codereview.chromium.org/491483006/diff/20001/public/web/WebFrame.h#ne... public/web/WebFrame.h:157: virtual void setInlineTextBoxAccessibilityEnabled(bool) = 0; These aren't needed.
You shouldn't need to touch any of the Web*Frame* classes. WebSettings should be sufficient.
https://codereview.chromium.org/491483006/diff/20001/public/web/WebFrame.h File public/web/WebFrame.h (right): https://codereview.chromium.org/491483006/diff/20001/public/web/WebFrame.h#ne... public/web/WebFrame.h:157: virtual void setInlineTextBoxAccessibilityEnabled(bool) = 0; On 2014/08/23 06:01:42, abarth wrote: > These aren't needed. Done.
On 2014/08/25 22:50:07, aboxhall wrote: > https://codereview.chromium.org/491483006/diff/20001/public/web/WebFrame.h > File public/web/WebFrame.h (right): > > https://codereview.chromium.org/491483006/diff/20001/public/web/WebFrame.h#ne... > public/web/WebFrame.h:157: virtual void > setInlineTextBoxAccessibilityEnabled(bool) = 0; > On 2014/08/23 06:01:42, abarth wrote: > > These aren't needed. > > Done. abarth@: PTAL?
lgtm
The CQ bit was checked by aboxhall@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/aboxhall@chromium.org/491483006/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as 181001 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
