Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
638003004:
Introduce WebFrameWidget to Blink (Closed)
|
Created:
6 years, 2 months ago by kenrb Modified:
5 years, 11 months ago CC:
dcheng, blink-reviews, bokan, dglazkov+blink, jamesr, mkwst+moarreviews_chromium.org, mlamouri (slow - plz ping), site-isolation-reviews_chromium.org Base URL:
svn://svn.chromium.org/blink/trunk Target Ref:
refs/remotes/origin/master Project:
blink Visibility:
Public. |
DescriptionIntroduce WebFrameWidget to Blink
To support out-of-process iframes, there is a need to associate multiple WebWidgets with a single frame tree in a process. This means that the WebViewImpl cannot be used as the WebWidget.
This CL creates a WebFrameWidget that can be associated with a WebFrame that is the root of a local frame tree. The goal is to eventually have this entirely replace the WebWidget implementation in WebViewImpl.
BUG=419087
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=187874
Patch Set 1 #Patch Set 2 : Bug fixes #Patch Set 3 : Rebased, cleaned up some #
Total comments: 32
Patch Set 4 : Review comments addressed #Patch Set 5 : More review updates #
Total comments: 17
Patch Set 6 : Rebase #Patch Set 7 : WebFrameWidget to take a WebLocalFrame on creation #
Total comments: 18
Patch Set 8 : More comments addressed #
Total comments: 11
Patch Set 9 : Moar review comments addressed #
Total comments: 2
Patch Set 10 : Style #
Messages
Total messages: 28 (5 generated)
kenrb@chromium.org changed reviewers: + dcheng@chromium.org, japhet@chromium.org
kenrb@chromium.org changed required reviewers: + dcheng@chromium.org, japhet@chromium.org
Test not up yet, but PTAL? There is a fair bit of code here that needs review.
https://codereview.chromium.org/638003004/diff/40001/Source/web/ChromeClientI... File Source/web/ChromeClientImpl.cpp (right): https://codereview.chromium.org/638003004/diff/40001/Source/web/ChromeClientI... Source/web/ChromeClientImpl.cpp:498: if (!WebLocalFrameImpl::fromFrame(localRoot)->frameWidget()) { Style nit: {} not necessary on this if-else https://codereview.chromium.org/638003004/diff/40001/Source/web/WebFrameWidge... File Source/web/WebFrameWidgetImpl.h (right): https://codereview.chromium.org/638003004/diff/40001/Source/web/WebFrameWidge... Source/web/WebFrameWidgetImpl.h:1: /* It looks like there's a bunch of stuff in these new files that are simply copied from WebViewImpl and which probably don't make sense in WebFrameWidgetImpl. Is the plan to just fork the class in this patch and fix all the nonsense in followup patches? https://codereview.chromium.org/638003004/diff/40001/public/web/WebFrameWidget.h File public/web/WebFrameWidget.h (right): https://codereview.chromium.org/638003004/diff/40001/public/web/WebFrameWidge... public/web/WebFrameWidget.h:39: class Page; Page shouldn't be forward declared in public/
https://codereview.chromium.org/638003004/diff/40001/Source/web/WebFrameWidge... File Source/web/WebFrameWidgetImpl.h (right): https://codereview.chromium.org/638003004/diff/40001/Source/web/WebFrameWidge... Source/web/WebFrameWidgetImpl.h:1: /* On 2014/12/10 19:19:03, Nate Chapin wrote: > It looks like there's a bunch of stuff in these new files that are simply copied > from WebViewImpl and which probably don't make sense in WebFrameWidgetImpl. Is > the plan to just fork the class in this patch and fix all the nonsense in > followup patches? The end goal is to make WebViewImpl no longer inherit WebWidget, and move all of its WebWidget implementation to this class. The forking is unfortunate but I couldn't find a cleaner way to do it. What specifically doesn't make sense in WebFrameWidgetImpl? I was trying to move over the minimum amount of code that would enable our current functionality for out-of-process iframes to continue working, and would be interested in knowing if you see code here that I can remove.
On 2014/12/10 19:59:27, kenrb wrote: > https://codereview.chromium.org/638003004/diff/40001/Source/web/WebFrameWidge... > File Source/web/WebFrameWidgetImpl.h (right): > > https://codereview.chromium.org/638003004/diff/40001/Source/web/WebFrameWidge... > Source/web/WebFrameWidgetImpl.h:1: /* > On 2014/12/10 19:19:03, Nate Chapin wrote: > > It looks like there's a bunch of stuff in these new files that are simply > copied > > from WebViewImpl and which probably don't make sense in WebFrameWidgetImpl. Is > > the plan to just fork the class in this patch and fix all the nonsense in > > followup patches? > > The end goal is to make WebViewImpl no longer inherit WebWidget, and move all of > its WebWidget implementation to this class. The forking is unfortunate but I > couldn't find a cleaner way to do it. What specifically doesn't make sense in > WebFrameWidgetImpl? I was trying to move over the minimum amount of code that > would enable our current functionality for out-of-process iframes to continue > working, and would be interested in knowing if you see code here that I can > remove. I skimmed it pretty quickly, but there were a couple functions with FIXMEs that might be dead code. Also, a page() accessor set off alarms in my head, but now that I think about it, that's probably just scaffolding to enable the move.
On 2014/12/10 20:04:17, Nate Chapin wrote: > On 2014/12/10 19:59:27, kenrb wrote: > > > https://codereview.chromium.org/638003004/diff/40001/Source/web/WebFrameWidge... > > File Source/web/WebFrameWidgetImpl.h (right): > > > > > https://codereview.chromium.org/638003004/diff/40001/Source/web/WebFrameWidge... > > Source/web/WebFrameWidgetImpl.h:1: /* > > On 2014/12/10 19:19:03, Nate Chapin wrote: > > > It looks like there's a bunch of stuff in these new files that are simply > > copied > > > from WebViewImpl and which probably don't make sense in WebFrameWidgetImpl. > Is > > > the plan to just fork the class in this patch and fix all the nonsense in > > > followup patches? > > > > The end goal is to make WebViewImpl no longer inherit WebWidget, and move all > of > > its WebWidget implementation to this class. The forking is unfortunate but I > > couldn't find a cleaner way to do it. What specifically doesn't make sense in > > WebFrameWidgetImpl? I was trying to move over the minimum amount of code that > > would enable our current functionality for out-of-process iframes to continue > > working, and would be interested in knowing if you see code here that I can > > remove. > > I skimmed it pretty quickly, but there were a couple functions with FIXMEs that > might be dead code. Also, a page() accessor set off alarms in my head, but now > that I think about it, that's probably just scaffolding to enable the move. Some of the FIXMEs are because there are pieces of WebViewImpl that will have to move over but haven't yet. There are a couple of cases of importing existing FIXMEs from WebViewImpl in code that does need to move. The page() accessor is used for many of the same things as WebViewImpl's page() accessor.
nick@chromium.org changed reviewers: + nick@chromium.org
https://codereview.chromium.org/638003004/diff/40001/Source/web/ChromeClientI... File Source/web/ChromeClientImpl.cpp (right): https://codereview.chromium.org/638003004/diff/40001/Source/web/ChromeClientI... Source/web/ChromeClientImpl.cpp:710: ASSERT(webFrame && webFrame->frameWidget()); After patching in this change, this ASSERT seems to fire in one of the browser_tests, causing a failure in the test. This was on Windows in a debug build. frameWidget() returns null. You can repro by running (and attaching a debugger to the child processes): browser_tests.exe --single_process --gtest_filter=TaskManagerOOPIFBrowserTest.KillSubframe/1 --wait-for-debugger-children The test is failing roughly at this point in time. https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/tas... Not sure if it's the b.com or c.com subframe process that's dying.
dcheng@chromium.org changed required reviewers: - dcheng@chromium.org, japhet@chromium.org
This is a pretty big patch, but some quick thoughts. https://codereview.chromium.org/638003004/diff/40001/Source/web/ChromeClientI... File Source/web/ChromeClientImpl.cpp (right): https://codereview.chromium.org/638003004/diff/40001/Source/web/ChromeClientI... Source/web/ChromeClientImpl.cpp:497: // If the frame is still being created, it might not yet have a WebWidget. I guess I don't understand--why are we scheduling animations before all our state is set up? That seems hard to reason about. https://codereview.chromium.org/638003004/diff/40001/Source/web/WebFrameWidge... File Source/web/WebFrameWidgetImpl.cpp (right): https://codereview.chromium.org/638003004/diff/40001/Source/web/WebFrameWidge... Source/web/WebFrameWidgetImpl.cpp:116: , m_localRoot(0) nullptr in new code. https://codereview.chromium.org/638003004/diff/40001/Source/web/WebFrameWidge... Source/web/WebFrameWidgetImpl.cpp:153: if (m_localRoot && m_localRoot->frameView()) Do you have some followup patches that show how we'll call setLocalRoot()? It seems weird that we have to handle the case where m_localRoot is null at all. https://codereview.chromium.org/638003004/diff/40001/Source/web/WebFrameWidge... File Source/web/WebFrameWidgetImpl.h (right): https://codereview.chromium.org/638003004/diff/40001/Source/web/WebFrameWidge... Source/web/WebFrameWidgetImpl.h:64: virtual void close() override; No virtual if using 'override'. https://codereview.chromium.org/638003004/diff/40001/Source/web/WebFrameWidge... Source/web/WebFrameWidgetImpl.h:116: WebWidgetClient* client() { return m_client; } const https://codereview.chromium.org/638003004/diff/40001/Source/web/WebFrameWidge... Source/web/WebFrameWidgetImpl.h:121: // WebFrameWidgetImpl functions: This comment isn't necessary--we don't generally annotate non-overrides, since we assume they belong to the class by default. https://codereview.chromium.org/638003004/diff/40001/Source/web/WebFrameWidge... Source/web/WebFrameWidgetImpl.h:216: WebPoint m_lastMouseDownPoint; I don't think this belongs on WebFrameWidget. https://codereview.chromium.org/638003004/diff/40001/Source/web/WebFrameWidge... Source/web/WebFrameWidgetImpl.h:221: WebColor m_backgroundColorOverride; We should only have one base background color per view right? Do we need to add this to WebFrameWidget? https://codereview.chromium.org/638003004/diff/40001/public/web/WebFrameWidget.h File public/web/WebFrameWidget.h (right): https://codereview.chromium.org/638003004/diff/40001/public/web/WebFrameWidge... public/web/WebFrameWidget.h:48: virtual void setLocalRoot(WebLocalFrame*) { } This is something we implement only inside Blink right? I think it's better to make this pure virtual.
The test still isn't up yet but I tried to address all comments in this patch set. https://codereview.chromium.org/638003004/diff/40001/Source/web/ChromeClientI... File Source/web/ChromeClientImpl.cpp (right): https://codereview.chromium.org/638003004/diff/40001/Source/web/ChromeClientI... Source/web/ChromeClientImpl.cpp:497: // If the frame is still being created, it might not yet have a WebWidget. On 2014/12/10 21:00:58, dcheng wrote: > I guess I don't understand--why are we scheduling animations before all our > state is set up? That seems hard to reason about. It is called during attachment of the Document's renderer, which is part of frame initialization. Not totally unreasonable imo. https://codereview.chromium.org/638003004/diff/40001/Source/web/ChromeClientI... Source/web/ChromeClientImpl.cpp:498: if (!WebLocalFrameImpl::fromFrame(localRoot)->frameWidget()) { On 2014/12/10 19:19:03, Nate Chapin wrote: > Style nit: {} not necessary on this if-else Done. https://codereview.chromium.org/638003004/diff/40001/Source/web/ChromeClientI... Source/web/ChromeClientImpl.cpp:710: ASSERT(webFrame && webFrame->frameWidget()); On 2014/12/10 20:16:06, ncarter wrote: > After patching in this change, this ASSERT seems to fire in one of the > browser_tests, causing a failure in the test. This was on Windows in a debug > build. frameWidget() returns null. You can repro by running (and attaching a > debugger to the child processes): > > browser_tests.exe --single_process > --gtest_filter=TaskManagerOOPIFBrowserTest.KillSubframe/1 > --wait-for-debugger-children > > The test is failing roughly at this point in time. > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/tas... > > Not sure if it's the b.com or c.com subframe process that's dying. This is an artifact of me not testing without the Chromium patch in place. :( I have added a temporary condition to catch this. https://codereview.chromium.org/638003004/diff/40001/Source/web/WebFrameWidge... File Source/web/WebFrameWidgetImpl.cpp (right): https://codereview.chromium.org/638003004/diff/40001/Source/web/WebFrameWidge... Source/web/WebFrameWidgetImpl.cpp:116: , m_localRoot(0) On 2014/12/10 21:00:58, dcheng wrote: > nullptr in new code. Done. https://codereview.chromium.org/638003004/diff/40001/Source/web/WebFrameWidge... Source/web/WebFrameWidgetImpl.cpp:153: if (m_localRoot && m_localRoot->frameView()) On 2014/12/10 21:00:58, dcheng wrote: > Do you have some followup patches that show how we'll call setLocalRoot()? It > seems weird that we have to handle the case where m_localRoot is null at all. It probably shouldn't, I have these checks in just for caution because WebViewImpl has NULL checks for mainFrameImpl(), but it is probably worth trying without them there. https://codereview.chromium.org/638003004/diff/40001/Source/web/WebFrameWidge... File Source/web/WebFrameWidgetImpl.h (right): https://codereview.chromium.org/638003004/diff/40001/Source/web/WebFrameWidge... Source/web/WebFrameWidgetImpl.h:64: virtual void close() override; On 2014/12/10 21:00:59, dcheng wrote: > No virtual if using 'override'. Where is that rule from? It isn't consistent with any of the other classes in Source/web, AFAICT. https://codereview.chromium.org/638003004/diff/40001/Source/web/WebFrameWidge... Source/web/WebFrameWidgetImpl.h:121: // WebFrameWidgetImpl functions: On 2014/12/10 21:00:59, dcheng wrote: > This comment isn't necessary--we don't generally annotate non-overrides, since > we assume they belong to the class by default. Done. https://codereview.chromium.org/638003004/diff/40001/Source/web/WebFrameWidge... Source/web/WebFrameWidgetImpl.h:216: WebPoint m_lastMouseDownPoint; On 2014/12/10 21:00:59, dcheng wrote: > I don't think this belongs on WebFrameWidget. Ok I have removed it, and the reference to it in handleMouseDown. I assume we will need to figure this out when we implement cross-process drag and drop. https://codereview.chromium.org/638003004/diff/40001/Source/web/WebFrameWidge... Source/web/WebFrameWidgetImpl.h:221: WebColor m_backgroundColorOverride; On 2014/12/10 21:00:59, dcheng wrote: > We should only have one base background color per view right? Do we need to add > this to WebFrameWidget? This is true, although in order to make this change I had to modify WebViewImpl to expose its backgroundColorOverride. Done. https://codereview.chromium.org/638003004/diff/40001/public/web/WebFrameWidget.h File public/web/WebFrameWidget.h (right): https://codereview.chromium.org/638003004/diff/40001/public/web/WebFrameWidge... public/web/WebFrameWidget.h:39: class Page; On 2014/12/10 19:19:03, Nate Chapin wrote: > Page shouldn't be forward declared in public/ Done. https://codereview.chromium.org/638003004/diff/40001/public/web/WebFrameWidge... public/web/WebFrameWidget.h:48: virtual void setLocalRoot(WebLocalFrame*) { } On 2014/12/10 21:00:59, dcheng wrote: > This is something we implement only inside Blink right? I think it's better to > make this pure virtual. I don't think that works. This header gets included by code in content, and they need to see a vtable.
I'm still taking a more detailed look, but some quick replies. https://codereview.chromium.org/638003004/diff/40001/Source/web/ChromeClientI... File Source/web/ChromeClientImpl.cpp (right): https://codereview.chromium.org/638003004/diff/40001/Source/web/ChromeClientI... Source/web/ChromeClientImpl.cpp:497: // If the frame is still being created, it might not yet have a WebWidget. On 2014/12/11 20:21:21, kenrb wrote: > On 2014/12/10 21:00:58, dcheng wrote: > > I guess I don't understand--why are we scheduling animations before all our > > state is set up? That seems hard to reason about. > > It is called during attachment of the Document's renderer, which is part of > frame initialization. Not totally unreasonable imo. Is this going to be temporary code? I'm OK with this on a temporary basis, but in the long run, local roots should have a frame widget before we call this code. https://codereview.chromium.org/638003004/diff/40001/Source/web/WebFrameWidge... File Source/web/WebFrameWidgetImpl.h (right): https://codereview.chromium.org/638003004/diff/40001/Source/web/WebFrameWidge... Source/web/WebFrameWidgetImpl.h:64: virtual void close() override; On 2014/12/11 20:21:21, kenrb wrote: > On 2014/12/10 21:00:59, dcheng wrote: > > No virtual if using 'override'. > > Where is that rule from? It isn't consistent with any of the other classes in > Source/web, AFAICT. https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/sjotlpNyF-8 New code should try to follow the Chromium convention, especially things like this, since we're not concerned about consistency with code in the same file. https://codereview.chromium.org/638003004/diff/40001/public/web/WebFrameWidget.h File public/web/WebFrameWidget.h (right): https://codereview.chromium.org/638003004/diff/40001/public/web/WebFrameWidge... public/web/WebFrameWidget.h:48: virtual void setLocalRoot(WebLocalFrame*) { } On 2014/12/11 20:21:22, kenrb wrote: > On 2014/12/10 21:00:59, dcheng wrote: > > This is something we implement only inside Blink right? I think it's better to > > make this pure virtual. > > I don't think that works. This header gets included by code in content, and they > need to see a vtable. Pure virtual is fine. WebFrame.h is composed of a lot of pure virtuals.
https://codereview.chromium.org/638003004/diff/40001/Source/web/ChromeClientI... File Source/web/ChromeClientImpl.cpp (right): https://codereview.chromium.org/638003004/diff/40001/Source/web/ChromeClientI... Source/web/ChromeClientImpl.cpp:497: // If the frame is still being created, it might not yet have a WebWidget. On 2014/12/11 23:31:45, dcheng wrote: > On 2014/12/11 20:21:21, kenrb wrote: > > On 2014/12/10 21:00:58, dcheng wrote: > > > I guess I don't understand--why are we scheduling animations before all our > > > state is set up? That seems hard to reason about. > > > > It is called during attachment of the Document's renderer, which is part of > > frame initialization. Not totally unreasonable imo. > > Is this going to be temporary code? I'm OK with this on a temporary basis, but > in the long run, local roots should have a frame widget before we call this > code. The call to ChromeClientImpl::scheduleAnimation() will eventually go away, but I don't know what would be involved in trying to make this not get called during Document creation. Why do you say this shouldn't happen? https://codereview.chromium.org/638003004/diff/40001/Source/web/WebFrameWidge... File Source/web/WebFrameWidgetImpl.h (right): https://codereview.chromium.org/638003004/diff/40001/Source/web/WebFrameWidge... Source/web/WebFrameWidgetImpl.h:64: virtual void close() override; On 2014/12/11 23:31:45, dcheng wrote: > On 2014/12/11 20:21:21, kenrb wrote: > > On 2014/12/10 21:00:59, dcheng wrote: > > > No virtual if using 'override'. > > > > Where is that rule from? It isn't consistent with any of the other classes in > > Source/web, AFAICT. > > https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/sjotlpNyF-8 > > New code should try to follow the Chromium convention, especially things like > this, since we're not concerned about consistency with code in the same file. Done. https://codereview.chromium.org/638003004/diff/40001/Source/web/WebFrameWidge... Source/web/WebFrameWidgetImpl.h:116: WebWidgetClient* client() { return m_client; } On 2014/12/10 21:00:59, dcheng wrote: > const Done.
https://codereview.chromium.org/638003004/diff/40001/Source/web/ChromeClientI... File Source/web/ChromeClientImpl.cpp (right): https://codereview.chromium.org/638003004/diff/40001/Source/web/ChromeClientI... Source/web/ChromeClientImpl.cpp:497: // If the frame is still being created, it might not yet have a WebWidget. On 2014/12/16 18:51:00, kenrb wrote: > On 2014/12/11 23:31:45, dcheng wrote: > > On 2014/12/11 20:21:21, kenrb wrote: > > > On 2014/12/10 21:00:58, dcheng wrote: > > > > I guess I don't understand--why are we scheduling animations before all > our > > > > state is set up? That seems hard to reason about. > > > > > > It is called during attachment of the Document's renderer, which is part of > > > frame initialization. Not totally unreasonable imo. > > > > Is this going to be temporary code? I'm OK with this on a temporary basis, but > > in the long run, local roots should have a frame widget before we call this > > code. > > The call to ChromeClientImpl::scheduleAnimation() will eventually go away, but I > don't know what would be involved in trying to make this not get called during > Document creation. Why do you say this shouldn't happen? A WebView is a WebWidget, so you can never have one without the other. So calling a WebWidget method on a WebView always works. I'm OK with this if it's temporary. However, I would like us to use the same model for WebFrameWidget. If a WebLocalFrame is a local root, it should always have a WebFrameWidget. It should never be the case that we create a local root WebLocalFrame and then attach the WebFrameWidget later. This helps simplify the rest of the code; we don't need to null check frame widget if we have a local root: we know it's there, by definition. https://codereview.chromium.org/638003004/diff/80001/Source/web/WebFrameWidge... File Source/web/WebFrameWidgetImpl.cpp (right): https://codereview.chromium.org/638003004/diff/80001/Source/web/WebFrameWidge... Source/web/WebFrameWidgetImpl.cpp:440: if (m_page) Why might page be null here and not elsewhere? Do we, in fact, need to clear page at some point? https://codereview.chromium.org/638003004/diff/80001/Source/web/WebFrameWidge... File Source/web/WebFrameWidgetImpl.h (right): https://codereview.chromium.org/638003004/diff/80001/Source/web/WebFrameWidge... Source/web/WebFrameWidgetImpl.h:203: WebViewImpl* m_webView; What's the story for lifetimes for Page/WebViewImpl and how they relate to WebFrameWidget? What happens when we detach? Do we need to null these pointers out at some point, or is there something that guarantees that we'll never try to call a dead object? https://codereview.chromium.org/638003004/diff/80001/Source/web/WebLocalFrame... File Source/web/WebLocalFrameImpl.cpp (right): https://codereview.chromium.org/638003004/diff/80001/Source/web/WebLocalFrame... Source/web/WebLocalFrameImpl.cpp:2005: void WebLocalFrameImpl::setFrameWidget(WebFrameWidgetImpl* frameWidget) Similar to my previous comment, I feel this should be folded into WebLocalFrameImpl. At WebLocalFrameImpl creation, we should always know if we need to create it with a widget or not. Instead of this setter, I'd prefer to see two factory methods: WebLocalFrame::create() and WebLocalFrame::createWithWidget(). This should avoid the need to check whether or not frame widget is null for a local root. https://codereview.chromium.org/638003004/diff/80001/Source/web/WebLocalFrame... File Source/web/WebLocalFrameImpl.h (right): https://codereview.chromium.org/638003004/diff/80001/Source/web/WebLocalFrame... Source/web/WebLocalFrameImpl.h:348: WebFrameWidgetImpl* m_frameWidget; Should WebLocalFrame own the WebFrameWidget? It seems like their lifetimes should be tied together. https://codereview.chromium.org/638003004/diff/80001/public/web/WebFrameWidget.h File public/web/WebFrameWidget.h (right): https://codereview.chromium.org/638003004/diff/80001/public/web/WebFrameWidge... public/web/WebFrameWidget.h:45: BLINK_EXPORT static WebFrameWidget* create(WebWidgetClient*, WebView*); Why does this take a WebView* instead of a WebLocalFrame*?
https://codereview.chromium.org/638003004/diff/80001/Source/web/WebFrameWidge... File Source/web/WebFrameWidgetImpl.cpp (right): https://codereview.chromium.org/638003004/diff/80001/Source/web/WebFrameWidge... Source/web/WebFrameWidgetImpl.cpp:440: if (m_page) On 2014/12/16 20:13:06, dcheng wrote: > Why might page be null here and not elsewhere? Do we, in fact, need to clear > page at some point? I reproduced the checks in WebViewImpl, even though the expectations might be a bit different. It seems the most conservative approach and without adequate test coverage it is hard to tell where bugs will come from. https://codereview.chromium.org/638003004/diff/80001/Source/web/WebFrameWidge... File Source/web/WebFrameWidgetImpl.h (right): https://codereview.chromium.org/638003004/diff/80001/Source/web/WebFrameWidge... Source/web/WebFrameWidgetImpl.h:203: WebViewImpl* m_webView; On 2014/12/16 20:13:06, dcheng wrote: > What's the story for lifetimes for Page/WebViewImpl and how they relate to > WebFrameWidget? What happens when we detach? Do we need to null these pointers > out at some point, or is there something that guarantees that we'll never try to > call a dead object? I might not understand the lifetime issues here -- is it possible for a frame to outlive its Page or WebViewImpl? Is it worth removing the pointer and replacing these with accessors that do lookups based on the localRoot? https://codereview.chromium.org/638003004/diff/80001/Source/web/WebLocalFrame... File Source/web/WebLocalFrameImpl.cpp (right): https://codereview.chromium.org/638003004/diff/80001/Source/web/WebLocalFrame... Source/web/WebLocalFrameImpl.cpp:2005: void WebLocalFrameImpl::setFrameWidget(WebFrameWidgetImpl* frameWidget) On 2014/12/16 20:13:06, dcheng wrote: > Similar to my previous comment, I feel this should be folded into > WebLocalFrameImpl. At WebLocalFrameImpl creation, we should always know if we > need to create it with a widget or not. Instead of this setter, I'd prefer to > see two factory methods: WebLocalFrame::create() and > WebLocalFrame::createWithWidget(). This should avoid the need to check whether > or not frame widget is null for a local root. I think this makes RenderWidget too complicated, which really needs a WebWidget to talk to. https://codereview.chromium.org/638003004/diff/80001/Source/web/WebLocalFrame... File Source/web/WebLocalFrameImpl.h (right): https://codereview.chromium.org/638003004/diff/80001/Source/web/WebLocalFrame... Source/web/WebLocalFrameImpl.h:348: WebFrameWidgetImpl* m_frameWidget; On 2014/12/16 20:13:06, dcheng wrote: > Should WebLocalFrame own the WebFrameWidget? It seems like their lifetimes > should be tied together. The model I had in mind was this: RenderFrame owns RenderWidget RenderFrame owns WebLocalFrame RenderWidget owns WebFrameWidget https://codereview.chromium.org/638003004/diff/80001/public/web/WebFrameWidget.h File public/web/WebFrameWidget.h (right): https://codereview.chromium.org/638003004/diff/80001/public/web/WebFrameWidge... public/web/WebFrameWidget.h:45: BLINK_EXPORT static WebFrameWidget* create(WebWidgetClient*, WebView*); On 2014/12/16 20:13:06, dcheng wrote: > Why does this take a WebView* instead of a WebLocalFrame*? See above.
https://codereview.chromium.org/638003004/diff/80001/Source/web/WebFrameWidge... File Source/web/WebFrameWidgetImpl.cpp (right): https://codereview.chromium.org/638003004/diff/80001/Source/web/WebFrameWidge... Source/web/WebFrameWidgetImpl.cpp:440: if (m_page) On 2014/12/16 22:04:12, kenrb wrote: > On 2014/12/16 20:13:06, dcheng wrote: > > Why might page be null here and not elsewhere? Do we, in fact, need to clear > > page at some point? > > I reproduced the checks in WebViewImpl, even though the expectations might be a > bit different. It seems the most conservative approach and without adequate test > coverage it is hard to tell where bugs will come from. Well, we don't currently ever null out the page. So it seems like these checks don't do anything. For the sake of making this review simpler, we can just FIXME the m_page/m_webView members for now and mention that we need to think more about WebFrameWidget shutdown, and handle it in a followup patch. https://codereview.chromium.org/638003004/diff/80001/Source/web/WebFrameWidge... File Source/web/WebFrameWidgetImpl.h (right): https://codereview.chromium.org/638003004/diff/80001/Source/web/WebFrameWidge... Source/web/WebFrameWidgetImpl.h:203: WebViewImpl* m_webView; On 2014/12/16 22:04:12, kenrb wrote: > On 2014/12/16 20:13:06, dcheng wrote: > > What's the story for lifetimes for Page/WebViewImpl and how they relate to > > WebFrameWidget? What happens when we detach? Do we need to null these pointers > > out at some point, or is there something that guarantees that we'll never try > to > > call a dead object? > > I might not understand the lifetime issues here -- is it possible for a frame to > outlive its Page or WebViewImpl? > > Is it worth removing the pointer and replacing these with accessors that do > lookups based on the localRoot? Well, today, WebView (and the WebWidget) own the page. The page is destroyed when we call WebViewImpl::close(). I don't think I've got a good enough overall picture from this patch to say whether or not we need an explicit shutdown path for WebFrameWidget as well. It's probably fine to leave it as it is for now and figure out lifetime issues as we go (frame detach/close is complicated enough as it is) https://codereview.chromium.org/638003004/diff/80001/Source/web/WebLocalFrame... File Source/web/WebLocalFrameImpl.cpp (right): https://codereview.chromium.org/638003004/diff/80001/Source/web/WebLocalFrame... Source/web/WebLocalFrameImpl.cpp:2005: void WebLocalFrameImpl::setFrameWidget(WebFrameWidgetImpl* frameWidget) On 2014/12/16 22:04:12, kenrb wrote: > On 2014/12/16 20:13:06, dcheng wrote: > > Similar to my previous comment, I feel this should be folded into > > WebLocalFrameImpl. At WebLocalFrameImpl creation, we should always know if we > > need to create it with a widget or not. Instead of this setter, I'd prefer to > > see two factory methods: WebLocalFrame::create() and > > WebLocalFrame::createWithWidget(). This should avoid the need to check whether > > or not frame widget is null for a local root. > > I think this makes RenderWidget too complicated, which really needs a WebWidget > to talk to. Do you have a Chromium patch that I could look at? If render widget doesn't hold a pointer to a WebFrameWidget, can't it just hold a back pointer to the RenderFrame it's in? It seems like it should be easy to get to the WebWidget from there. https://codereview.chromium.org/638003004/diff/80001/Source/web/WebLocalFrame... File Source/web/WebLocalFrameImpl.h (right): https://codereview.chromium.org/638003004/diff/80001/Source/web/WebLocalFrame... Source/web/WebLocalFrameImpl.h:348: WebFrameWidgetImpl* m_frameWidget; On 2014/12/16 22:04:12, kenrb wrote: > On 2014/12/16 20:13:06, dcheng wrote: > > Should WebLocalFrame own the WebFrameWidget? It seems like their lifetimes > > should be tied together. > > The model I had in mind was this: > RenderFrame owns RenderWidget > RenderFrame owns WebLocalFrame > RenderWidget owns WebFrameWidget I think this is problematic. It's fine for RenderFrame to own a RenderWidget. I don't think we should have parallel ownership hierarchies for WebLocalFrame and WebFrameWidget though. The WebLocalFrame should own its WebFrameWidget if it has one. I think this is important because we're exposing WebLocalFrame/WebFrameWidget from the Blink API layer. Having setFrameWidget() and setLocalRoot() exposes this internal state management to content, which is something I'd prefer not to do. At least from what I can tell, this should lead to a pretty straightforward API: - WebRemoteFrame::createLocalFrame() always needs to call the WebLocalFrame::createWithWidget() overload. - content::RenderViewImpl (or whatever content class ends up responsible for creating the main frame) always calls WebLocalFrame::createWithWidget(). - content::RenderFrameImpl::createChildFrame() always uses WebLocalFrame::create().
If we need more discussion on the ownership models here, we can maybe have a chat this afternoon to sort this out. https://codereview.chromium.org/638003004/diff/80001/Source/web/WebFrameWidge... File Source/web/WebFrameWidgetImpl.cpp (right): https://codereview.chromium.org/638003004/diff/80001/Source/web/WebFrameWidge... Source/web/WebFrameWidgetImpl.cpp:440: if (m_page) On 2014/12/16 23:20:20, dcheng wrote: > On 2014/12/16 22:04:12, kenrb wrote: > > On 2014/12/16 20:13:06, dcheng wrote: > > > Why might page be null here and not elsewhere? Do we, in fact, need to clear > > > page at some point? > > > > I reproduced the checks in WebViewImpl, even though the expectations might be > a > > bit different. It seems the most conservative approach and without adequate > test > > coverage it is hard to tell where bugs will come from. > > Well, we don't currently ever null out the page. So it seems like these checks > don't do anything. For the sake of making this review simpler, we can just FIXME > the m_page/m_webView members for now and mention that we need to think more > about WebFrameWidget shutdown, and handle it in a followup patch. Sounds good. https://codereview.chromium.org/638003004/diff/80001/Source/web/WebFrameWidge... File Source/web/WebFrameWidgetImpl.h (right): https://codereview.chromium.org/638003004/diff/80001/Source/web/WebFrameWidge... Source/web/WebFrameWidgetImpl.h:203: WebViewImpl* m_webView; On 2014/12/16 23:20:20, dcheng wrote: > On 2014/12/16 22:04:12, kenrb wrote: > > On 2014/12/16 20:13:06, dcheng wrote: > > > What's the story for lifetimes for Page/WebViewImpl and how they relate to > > > WebFrameWidget? What happens when we detach? Do we need to null these > pointers > > > out at some point, or is there something that guarantees that we'll never > try > > to > > > call a dead object? > > > > I might not understand the lifetime issues here -- is it possible for a frame > to > > outlive its Page or WebViewImpl? > > > > Is it worth removing the pointer and replacing these with accessors that do > > lookups based on the localRoot? > > Well, today, WebView (and the WebWidget) own the page. The page is destroyed > when we call WebViewImpl::close(). I don't think I've got a good enough overall > picture from this patch to say whether or not we need an explicit shutdown path > for WebFrameWidget as well. It's probably fine to leave it as it is for now and > figure out lifetime issues as we go (frame detach/close is complicated enough as > it is) This is fine, I can add FIXMEs for the open questions. https://codereview.chromium.org/638003004/diff/80001/Source/web/WebLocalFrame... File Source/web/WebLocalFrameImpl.h (right): https://codereview.chromium.org/638003004/diff/80001/Source/web/WebLocalFrame... Source/web/WebLocalFrameImpl.h:348: WebFrameWidgetImpl* m_frameWidget; On 2014/12/16 23:20:20, dcheng wrote: > On 2014/12/16 22:04:12, kenrb wrote: > > On 2014/12/16 20:13:06, dcheng wrote: > > > Should WebLocalFrame own the WebFrameWidget? It seems like their lifetimes > > > should be tied together. > > > > The model I had in mind was this: > > RenderFrame owns RenderWidget > > RenderFrame owns WebLocalFrame > > RenderWidget owns WebFrameWidget > > I think this is problematic. It's fine for RenderFrame to own a RenderWidget. I > don't think we should have parallel ownership hierarchies for WebLocalFrame and > WebFrameWidget though. The WebLocalFrame should own its WebFrameWidget if it has > one. > > I think this is important because we're exposing WebLocalFrame/WebFrameWidget > from the Blink API layer. Having setFrameWidget() and setLocalRoot() exposes > this internal state management to content, which is something I'd prefer not to > do. > > At least from what I can tell, this should lead to a pretty straightforward API: > - WebRemoteFrame::createLocalFrame() always needs to call the > WebLocalFrame::createWithWidget() overload. > - content::RenderViewImpl (or whatever content class ends up responsible for > creating the main frame) always calls WebLocalFrame::createWithWidget(). > - content::RenderFrameImpl::createChildFrame() always uses > WebLocalFrame::create(). I see what you are saying, but my concern is that it creates inconsistency in the Blink API leading to more complex content code. Currently there are 2 kinds WebWidget, and in both cases it is owned by RenderWidget (WebView/WebWidget, and WebPagePopupImpl -- the first of those will go away eventually but the second will not). RenderWidget has a pointer to a WebWidget and it would create inter-layer complexity to have to differentiate between owned and non-owned WebWidgets. The Chromium patch is here: https://codereview.chromium.org/616133002/
Daniel, Nate: PTAL? I tried to capture what Daniel and I talked about this afternoon. WebFrameWidget creation is a little simpler now, but ownership relationships are left as they are with FIXMEs to think through if we can make it cleaner in future.
https://codereview.chromium.org/638003004/diff/120001/Source/web/ChromeClient... File Source/web/ChromeClientImpl.cpp (right): https://codereview.chromium.org/638003004/diff/120001/Source/web/ChromeClient... Source/web/ChromeClientImpl.cpp:499: // a local frame root that doesn't have a WebWidget? Nit: expand this comment that it's OK to do this because there's nothing to draw yet or something. https://codereview.chromium.org/638003004/diff/120001/Source/web/WebFrameWidg... File Source/web/WebFrameWidgetImpl.cpp (right): https://codereview.chromium.org/638003004/diff/120001/Source/web/WebFrameWidg... Source/web/WebFrameWidgetImpl.cpp:34: #include "core/editing/Editor.h" Let's clean up some of these includes. At first glance, it looks like we're not using FrameHost.h, TraveEvent.h, WebActiveWheelFlingParameters.h, WebAutofillClient.h, WebFrameClient.h, WebHitTestResult.h, WebInputElement.h, WebNode.h, WebMediaPlayerAction.h, WebPlugin.h, WebPluginAction.h, WebRange.h, WebViewClient.h, WebWindowFeatures.h, CompositionUnderlineVectorBuilder.h, ContextFeaturesClientImpl.h, DatabaseClientImpl.h, FullscreenController.h, GraphicsLayerFactoryChromium.h, LinkHighlight.h, NavigatorContentUtilsClientImpl.h, PrerendererClientImpl.h, SpeechRecognitionClientProxy.h, StorageQuotaClientImpl.h, ValidationMessageClientImpl.h, ViewportAnchor.h, WebDevToolsAgentImpl.h, WebDevToolsAgentPrivate.h, WebSettingsImpl.h, WorkerGlobalScopeProxyProviderImpl.h, and CurrentTime.h. I'm probably wrong about some of these, but most of them don't appear to be used atm. https://codereview.chromium.org/638003004/diff/120001/Source/web/WebFrameWidg... Source/web/WebFrameWidgetImpl.cpp:110: , m_localRoot(toWebLocalFrameImpl(frame)) Let's be consistent with the naming--either m_localRoot and taking an argument named localRoot or m_frame and taking an argument named frame. Personally, I prefer the latter. https://codereview.chromium.org/638003004/diff/120001/Source/web/WebFrameWidg... Source/web/WebFrameWidgetImpl.cpp:120: { ASSERT(m_frame->isLocalRoot()) https://codereview.chromium.org/638003004/diff/120001/Source/web/WebFrameWidg... Source/web/WebFrameWidgetImpl.cpp:132: { We might want to clear our reference to m_frame as well, but let's do that in a future patch. https://codereview.chromium.org/638003004/diff/120001/Source/web/WebFrameWidg... Source/web/WebFrameWidgetImpl.cpp:135: m_client = 0; Nit: nullptr https://codereview.chromium.org/638003004/diff/120001/Source/web/WebFrameWidg... File Source/web/WebFrameWidgetImpl.h (right): https://codereview.chromium.org/638003004/diff/120001/Source/web/WebFrameWidg... Source/web/WebFrameWidgetImpl.h:58: class WebFrameWidgetImpl : public WebFrameWidget class WebFrameWidgetImpl final https://codereview.chromium.org/638003004/diff/120001/Source/web/WebFrameWidg... Source/web/WebFrameWidgetImpl.h:125: void hasTouchEventHandlers(bool); Nit: setHasTouchEventHandlers() https://codereview.chromium.org/638003004/diff/120001/Source/web/WebFrameWidg... Source/web/WebFrameWidgetImpl.h:140: Page* page() const Nit: collapse this into one line.
https://codereview.chromium.org/638003004/diff/120001/Source/web/ChromeClient... File Source/web/ChromeClientImpl.cpp (right): https://codereview.chromium.org/638003004/diff/120001/Source/web/ChromeClient... Source/web/ChromeClientImpl.cpp:499: // a local frame root that doesn't have a WebWidget? On 2014/12/17 23:22:53, dcheng wrote: > Nit: expand this comment that it's OK to do this because there's nothing to draw > yet or something. Done. https://codereview.chromium.org/638003004/diff/120001/Source/web/WebFrameWidg... File Source/web/WebFrameWidgetImpl.cpp (right): https://codereview.chromium.org/638003004/diff/120001/Source/web/WebFrameWidg... Source/web/WebFrameWidgetImpl.cpp:34: #include "core/editing/Editor.h" On 2014/12/17 23:22:53, dcheng wrote: > Let's clean up some of these includes. At first glance, it looks like we're not > using FrameHost.h, TraveEvent.h, WebActiveWheelFlingParameters.h, > WebAutofillClient.h, WebFrameClient.h, WebHitTestResult.h, WebInputElement.h, > WebNode.h, WebMediaPlayerAction.h, WebPlugin.h, WebPluginAction.h, WebRange.h, > WebViewClient.h, WebWindowFeatures.h, CompositionUnderlineVectorBuilder.h, > ContextFeaturesClientImpl.h, DatabaseClientImpl.h, FullscreenController.h, > GraphicsLayerFactoryChromium.h, LinkHighlight.h, > NavigatorContentUtilsClientImpl.h, PrerendererClientImpl.h, > SpeechRecognitionClientProxy.h, StorageQuotaClientImpl.h, > ValidationMessageClientImpl.h, ViewportAnchor.h, WebDevToolsAgentImpl.h, > WebDevToolsAgentPrivate.h, WebSettingsImpl.h, > WorkerGlobalScopeProxyProviderImpl.h, and CurrentTime.h. I'm probably wrong > about some of these, but most of them don't appear to be used atm. Done. I believe the updated set is minimal. https://codereview.chromium.org/638003004/diff/120001/Source/web/WebFrameWidg... Source/web/WebFrameWidgetImpl.cpp:110: , m_localRoot(toWebLocalFrameImpl(frame)) On 2014/12/17 23:22:53, dcheng wrote: > Let's be consistent with the naming--either m_localRoot and taking an argument > named localRoot or m_frame and taking an argument named frame. > > Personally, I prefer the latter. Going with the former, I think it makes it a bit clearer. WebViewImpl commonly references mainFrame rather than just frame, for a similar reason. https://codereview.chromium.org/638003004/diff/120001/Source/web/WebFrameWidg... Source/web/WebFrameWidgetImpl.cpp:120: { On 2014/12/17 23:22:53, dcheng wrote: > ASSERT(m_frame->isLocalRoot()) Done. https://codereview.chromium.org/638003004/diff/120001/Source/web/WebFrameWidg... Source/web/WebFrameWidgetImpl.cpp:132: { On 2014/12/17 23:22:53, dcheng wrote: > We might want to clear our reference to m_frame as well, but let's do that in a > future patch. Acknowledged. https://codereview.chromium.org/638003004/diff/120001/Source/web/WebFrameWidg... Source/web/WebFrameWidgetImpl.cpp:135: m_client = 0; On 2014/12/17 23:22:53, dcheng wrote: > Nit: nullptr Done. https://codereview.chromium.org/638003004/diff/120001/Source/web/WebFrameWidg... File Source/web/WebFrameWidgetImpl.h (right): https://codereview.chromium.org/638003004/diff/120001/Source/web/WebFrameWidg... Source/web/WebFrameWidgetImpl.h:58: class WebFrameWidgetImpl : public WebFrameWidget On 2014/12/17 23:22:53, dcheng wrote: > class WebFrameWidgetImpl final Done. https://codereview.chromium.org/638003004/diff/120001/Source/web/WebFrameWidg... Source/web/WebFrameWidgetImpl.h:125: void hasTouchEventHandlers(bool); On 2014/12/17 23:22:53, dcheng wrote: > Nit: setHasTouchEventHandlers() I removed it instead. That was from WebViewImpl but at this point it isn't needed. https://codereview.chromium.org/638003004/diff/120001/Source/web/WebFrameWidg... Source/web/WebFrameWidgetImpl.h:140: Page* page() const On 2014/12/17 23:22:53, dcheng wrote: > Nit: collapse this into one line. Done.
lgtm with some nits. https://codereview.chromium.org/638003004/diff/140001/Source/web/WebFrameWidg... File Source/web/WebFrameWidgetImpl.cpp (right): https://codereview.chromium.org/638003004/diff/140001/Source/web/WebFrameWidg... Source/web/WebFrameWidgetImpl.cpp:294: const WebInputEvent* WebFrameWidgetImpl::m_currentInputEvent = 0; nullptr https://codereview.chromium.org/638003004/diff/140001/Source/web/WebFrameWidg... Source/web/WebFrameWidgetImpl.cpp:647: m_layerTreeView = 0; nullptr https://codereview.chromium.org/638003004/diff/140001/Source/web/WebFrameWidg... Source/web/WebFrameWidgetImpl.cpp:903: return m_page ? m_page->focusController().focusedOrMainFrame() : 0; s/0/nullptr/g from here to the end of the file https://codereview.chromium.org/638003004/diff/140001/Source/web/WebFrameWidg... File Source/web/WebFrameWidgetImpl.h (right): https://codereview.chromium.org/638003004/diff/140001/Source/web/WebFrameWidg... Source/web/WebFrameWidgetImpl.h:207: WebColor m_backgroundColorOverride; I think these are unreferenced now, so let's remove them. https://codereview.chromium.org/638003004/diff/140001/Source/web/WebFrameWidg... Source/web/WebFrameWidgetImpl.h:207: WebColor m_backgroundColorOverride; I think these are unreferenced now, so let's remove them. https://codereview.chromium.org/638003004/diff/140001/Source/web/WebLocalFram... File Source/web/WebLocalFrameImpl.h (right): https://codereview.chromium.org/638003004/diff/140001/Source/web/WebLocalFram... Source/web/WebLocalFrameImpl.h:322: WebFrameWidgetImpl* frameWidget(); const.
japhet@: Friendly ping for review? https://codereview.chromium.org/638003004/diff/140001/Source/web/WebFrameWidg... File Source/web/WebFrameWidgetImpl.cpp (right): https://codereview.chromium.org/638003004/diff/140001/Source/web/WebFrameWidg... Source/web/WebFrameWidgetImpl.cpp:294: const WebInputEvent* WebFrameWidgetImpl::m_currentInputEvent = 0; On 2014/12/18 19:52:43, dcheng wrote: > nullptr Done. https://codereview.chromium.org/638003004/diff/140001/Source/web/WebFrameWidg... Source/web/WebFrameWidgetImpl.cpp:647: m_layerTreeView = 0; On 2014/12/18 19:52:43, dcheng wrote: > nullptr Done. https://codereview.chromium.org/638003004/diff/140001/Source/web/WebFrameWidg... Source/web/WebFrameWidgetImpl.cpp:903: return m_page ? m_page->focusController().focusedOrMainFrame() : 0; On 2014/12/18 19:52:43, dcheng wrote: > s/0/nullptr/g from here to the end of the file Done, except presubmit checks reject "!= nullptr", so just passing pointer as boolean. https://codereview.chromium.org/638003004/diff/140001/Source/web/WebFrameWidg... File Source/web/WebFrameWidgetImpl.h (right): https://codereview.chromium.org/638003004/diff/140001/Source/web/WebFrameWidg... Source/web/WebFrameWidgetImpl.h:207: WebColor m_backgroundColorOverride; On 2014/12/18 19:52:43, dcheng wrote: > I think these are unreferenced now, so let's remove them. Done. https://codereview.chromium.org/638003004/diff/140001/Source/web/WebLocalFram... File Source/web/WebLocalFrameImpl.h (right): https://codereview.chromium.org/638003004/diff/140001/Source/web/WebLocalFram... Source/web/WebLocalFrameImpl.h:322: WebFrameWidgetImpl* frameWidget(); On 2014/12/18 19:52:43, dcheng wrote: > const. Done.
LGTM https://codereview.chromium.org/638003004/diff/160001/Source/web/WebFrameWidg... File Source/web/WebFrameWidgetImpl.cpp (right): https://codereview.chromium.org/638003004/diff/160001/Source/web/WebFrameWidg... Source/web/WebFrameWidgetImpl.cpp:168: // TODO: Implement pinch viewport for out-of-process iframes. Nit: TODO->FIXME
The CQ bit was checked by kenrb@chromium.org
https://codereview.chromium.org/638003004/diff/160001/Source/web/WebFrameWidg... File Source/web/WebFrameWidgetImpl.cpp (right): https://codereview.chromium.org/638003004/diff/160001/Source/web/WebFrameWidg... Source/web/WebFrameWidgetImpl.cpp:168: // TODO: Implement pinch viewport for out-of-process iframes. On 2015/01/05 21:36:09, Nate Chapin wrote: > Nit: TODO->FIXME Done.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/638003004/180001
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as https://src.chromium.org/viewvc/blink?view=rev&revision=187874 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
