Make the embedder responsible for creating the WebFrame

Make the embedder responsible for creating the WebFrame.

Previously, the WebViewImpl created the mainframe, and the WebFrameImpl created the child frames. This CL moves responsibility for creating all WebFrameImpls to the embedder.

WebFrames are now allocated using the create() factory method. The caller is responsible for ensuring WebFrame::close() is called to balance out the reference count.  This parallels the lifetime of WebView.

For the main frame, the embedder constructs the WebFrame first, and then provides the WebView a references to the main frame via the WebView::setMainFrame() method.

For child frames, the embedder provides the new WebFrame via WebFrameClient::createChildFrame().  The embedder is notified that the WebFrame is no longer needed via the WebFrameClient::frameDetached() call.

Because WebView no longer creates its own main frame, a WebViewHelper class has been added to reduce boilerplate in unittests.

Lastly, there is still some vestigial code left (all labeled with FIXMEs) to facilitate migration of embedders to the new lifetime semantics.  Those will be removed in a subsequent CL.

(https://codereview.chromium.org/23841002/ is the Chrome side of the CL)

BUG=245126
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=158354

[darin (slow to review), 2013-09-05 06:07:53]

I like what you are doing here. It might be nice if didCreateFrame was named
createFrame or createChildFrame instead.

https://codereview.chromium.org/23506013/diff/6001/public/web/WebFrameClient.h
File public/web/WebFrameClient.h (right):

https://codereview.chromium.org/23506013/diff/6001/public/web/WebFrameClient....
public/web/WebFrameClient.h:111: virtual WebFrame* didCreateFrame(WebString) {
return 0; }
note: it may seem as though blink / webkit coding style is to leave all
parameters unnamed, but actually you should give this one a name. the style rule
is to leave parameters unnamed when the best name for the parameter is the type
name. in this case, the name of the parameter is probably "name" or "frameName".

[awong, 2013-09-13 01:37:53]

I've made it so the embedder manages the lifetime.  Now

1) For the mainframe WebFrameImpl, the lifetime is RenderViewImpl::Initialize()
to RenderViewImpl::Close().
2) For child WebFrameImpl, the lifetime is RenderFrameImpl::createChildFrame()
to RenderFrameImpl::frameDetached()
(3) In unittests, I've added a FrameTestHelper::WebViewHelper to reduce
boilerplate for lifetime management of a WebView for test.

I'm still migrating some unittests. Let me know if this is the right direction.
If so, I think this all works and the next step will be cleaning this CL up and
figuring out how to land the patch.

[darin (slow to review), 2013-09-13 04:02:47]

Wow, good stuff!

https://codereview.chromium.org/23506013/diff/21001/Source/core/loader/FrameL...
File Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/23506013/diff/21001/Source/core/loader/FrameL...
Source/core/loader/FrameLoader.cpp:1161: // Note...the problem is mainframe
ownership is the embedder and not the
nit: this comment could use some word-smithing.

https://codereview.chromium.org/23506013/diff/21001/Source/web/WebFrameImpl.cpp
File Source/web/WebFrameImpl.cpp (right):

https://codereview.chromium.org/23506013/diff/21001/Source/web/WebFrameImpl.c...
Source/web/WebFrameImpl.cpp:536: void WebFrameImpl::close()
not that you should clean this up now, but WebFrameClient has a strange method
named willClose() that has no relation to this new method or
WebFrameClient::frameDetached(). it actually corresponds to the document for the
frame changing to another document. willClose means "the old document is being
closed."

https://codereview.chromium.org/23506013/diff/21001/Source/web/WebFrameImpl.c...
Source/web/WebFrameImpl.cpp:539: deref();  // Balances ref() acquired in
WebFrame::create
nit: WebFrameImpl::create

https://codereview.chromium.org/23506013/diff/21001/Source/web/WebFrameImpl.c...
Source/web/WebFrameImpl.cpp:540: }
you might want to add a comment here explaining what this function is doing. you
might explain what it means for the embedder to signal that it has closed the
WebFrame, etc.

https://codereview.chromium.org/23506013/diff/21001/Source/web/WebFrameImpl.c...
Source/web/WebFrameImpl.cpp:2098: return adoptRef(new WebFrameImpl(client,
identifier)).leakRef();
nit: add a comment indicating that this leak gets cleaned up in
WebFrameImpl::close()

https://codereview.chromium.org/23506013/diff/21001/Source/web/WebFrameImpl.c...
Source/web/WebFrameImpl.cpp:2118: , m_identifier(identifier)
We should just move the concept of WebFrame identifier to Chromium. it doesn't
need to be part of Blink at all.  it was a mistake that it was ever put here.
this identifier can be a property of the content::RenderFrameImpl on the
Chromium side.

https://codereview.chromium.org/23506013/diff/21001/Source/web/WebFrameImpl.c...
Source/web/WebFrameImpl.cpp:2182: m_client->frameDetached(webframe);
there have been bugs where it appeared from the chromium side that frameDetached
was not called when it should have been. i wonder if you found the culprit to
those bugs.

https://codereview.chromium.org/23506013/diff/21001/Source/web/WebHelperPlugi...
File Source/web/WebHelperPluginImpl.cpp (right):

https://codereview.chromium.org/23506013/diff/21001/Source/web/WebHelperPlugi...
Source/web/WebHelperPluginImpl.cpp:153: m_mainFrame =
static_cast<WebFrameImpl*>(WebFrame::create(client));
perhaps this guy should just use WebFrameImpl::create ?

https://codereview.chromium.org/23506013/diff/21001/Source/web/tests/FrameTes...
File Source/web/tests/FrameTestHelpers.cpp (right):

https://codereview.chromium.org/23506013/diff/21001/Source/web/tests/FrameTes...
Source/web/tests/FrameTestHelpers.cpp:115: m_mainFrame =
WebFrame::create(webFrameClient);
nit: allocate m_mainFrame closer to where it is first used?

https://codereview.chromium.org/23506013/diff/21001/public/web/WebFrame.h
File public/web/WebFrame.h (right):

https://codereview.chromium.org/23506013/diff/21001/public/web/WebFrame.h#new...
public/web/WebFrame.h:130: virtual void close() = 0;
nit: two new lines below, one new line above.

https://codereview.chromium.org/23506013/diff/21001/public/web/WebView.h
File public/web/WebView.h (right):

https://codereview.chromium.org/23506013/diff/21001/public/web/WebView.h#newc...
public/web/WebView.h:101: virtual void initializeMainFrame(WebFrame*) = 0;
Perhaps this method should be renamed to setMainFrame? The comments should
probably be updated accordingly too.

[awong, 2013-09-13 04:25:02]

Thanks for the comments!  I'll address most of them tomorrow. However, I wanted
to respond to the WebFrame identifier piece.

https://codereview.chromium.org/23506013/diff/21001/Source/web/WebFrameImpl.cpp
File Source/web/WebFrameImpl.cpp (right):

https://codereview.chromium.org/23506013/diff/21001/Source/web/WebFrameImpl.c...
Source/web/WebFrameImpl.cpp:2118: , m_identifier(identifier)
On 2013/09/13 04:02:47, darin wrote:
> We should just move the concept of WebFrame identifier to Chromium. it doesn't
> need to be part of Blink at all.  it was a mistake that it was ever put here.
> this identifier can be a property of the content::RenderFrameImpl on the
> Chromium side.

I did this initially, but didn't know how to handle WebSharedWorkerImpl.cpp and
WebHelperPluginImpl.cpp. Both these create a WebFrameImpl outside the purview of
RenderFrameImpl.

WebHelplerPluginImpl in particular uses the mainframe's render_frame_impl as the
client.  I have no idea whether or not WebHelperPluginImpl executes resource
load requests.  If it does, then I think it needs to have a unique WebFrame
identifier which means I can't move the WebFrame identifier into render_view
just yet.

Do you know whether it'd be okay for WebHelperPluginImpl to not have a sensible
WebFrame identifier?

Further more, since the per-frame RenderFrameHostImpl creation is sitting behind
a flag, I can't make this transition yet.  Otherwise, I think I would break
resource loads on trunk. :'(

[darin (slow to review), 2013-09-13 04:32:20]

I'd need to study WebSharedWorkerImpl.cpp and WebHelperPluginImpl.cpp some
more. The frame identifier is unlikely to matter for either, but I'd need
to confirm. I'm not so familiar with the latter.

-Darin


On Thu, Sep 12, 2013 at 9:25 PM, <ajwong@chromium.org> wrote:

> Thanks for the comments!  I'll address most of them tomorrow. However, I
> wanted
> to respond to the WebFrame identifier piece.
>
>
>
> https://codereview.chromium.**org/23506013/diff/21001/**
>
Source/web/WebFrameImpl.cpp<https://codereview.chromium.org/23506013/diff/21001/Source/web/WebFrameImpl.cpp>
> File Source/web/WebFrameImpl.cpp (right):
>
> https://codereview.chromium.**org/23506013/diff/21001/**
>
Source/web/WebFrameImpl.cpp#**newcode2118<https://codereview.chromium.org/23506013/diff/21001/Source/web/WebFrameImpl.cpp#newcode2118>
> Source/web/WebFrameImpl.cpp:**2118: , m_identifier(identifier)
> On 2013/09/13 04:02:47, darin wrote:
>
>> We should just move the concept of WebFrame identifier to Chromium. it
>>
> doesn't
>
>> need to be part of Blink at all.  it was a mistake that it was ever
>>
> put here.
>
>> this identifier can be a property of the content::RenderFrameImpl on
>>
> the
>
>> Chromium side.
>>
>
> I did this initially, but didn't know how to handle
> WebSharedWorkerImpl.cpp and WebHelperPluginImpl.cpp. Both these create a
> WebFrameImpl outside the purview of RenderFrameImpl.
>
> WebHelplerPluginImpl in particular uses the mainframe's
> render_frame_impl as the client.  I have no idea whether or not
> WebHelperPluginImpl executes resource load requests.  If it does, then I
> think it needs to have a unique WebFrame identifier which means I can't
> move the WebFrame identifier into render_view just yet.
>
> Do you know whether it'd be okay for WebHelperPluginImpl to not have a
> sensible WebFrame identifier?
>
> Further more, since the per-frame RenderFrameHostImpl creation is
> sitting behind a flag, I can't make this transition yet.  Otherwise, I
> think I would break resource loads on trunk. :'(
>
>
https://codereview.chromium.**org/23506013/<https://codereview.chromium.org/2...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[darin (slow to review), 2013-09-13 04:32:48]

I think the frame identifier is used by the Chrome Extension system in its
WebRequest API.

-Darin


On Thu, Sep 12, 2013 at 9:32 PM, Darin Fisher <darin@chromium.org> wrote:

> I'd need to study WebSharedWorkerImpl.cpp and WebHelperPluginImpl.cpp some
> more. The frame identifier is unlikely to matter for either, but I'd need
> to confirm. I'm not so familiar with the latter.
>
> -Darin
>
>
> On Thu, Sep 12, 2013 at 9:25 PM, <ajwong@chromium.org> wrote:
>
>> Thanks for the comments!  I'll address most of them tomorrow. However, I
>> wanted
>> to respond to the WebFrame identifier piece.
>>
>>
>>
>> https://codereview.chromium.**org/23506013/diff/21001/**
>>
Source/web/WebFrameImpl.cpp<https://codereview.chromium.org/23506013/diff/21001/Source/web/WebFrameImpl.cpp>
>> File Source/web/WebFrameImpl.cpp (right):
>>
>> https://codereview.chromium.**org/23506013/diff/21001/**
>>
Source/web/WebFrameImpl.cpp#**newcode2118<https://codereview.chromium.org/23506013/diff/21001/Source/web/WebFrameImpl.cpp#newcode2118>
>> Source/web/WebFrameImpl.cpp:**2118: , m_identifier(identifier)
>> On 2013/09/13 04:02:47, darin wrote:
>>
>>> We should just move the concept of WebFrame identifier to Chromium. it
>>>
>> doesn't
>>
>>> need to be part of Blink at all.  it was a mistake that it was ever
>>>
>> put here.
>>
>>> this identifier can be a property of the content::RenderFrameImpl on
>>>
>> the
>>
>>> Chromium side.
>>>
>>
>> I did this initially, but didn't know how to handle
>> WebSharedWorkerImpl.cpp and WebHelperPluginImpl.cpp. Both these create a
>> WebFrameImpl outside the purview of RenderFrameImpl.
>>
>> WebHelplerPluginImpl in particular uses the mainframe's
>> render_frame_impl as the client.  I have no idea whether or not
>> WebHelperPluginImpl executes resource load requests.  If it does, then I
>> think it needs to have a unique WebFrame identifier which means I can't
>> move the WebFrame identifier into render_view just yet.
>>
>> Do you know whether it'd be okay for WebHelperPluginImpl to not have a
>> sensible WebFrame identifier?
>>
>> Further more, since the per-frame RenderFrameHostImpl creation is
>> sitting behind a flag, I can't make this transition yet.  Otherwise, I
>> think I would break resource loads on trunk. :'(
>>
>>
https://codereview.chromium.**org/23506013/<https://codereview.chromium.org/2...
>>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[awong, 2013-09-13 04:46:08]

I'll look at it too, but even if it's okay to ignore them for
WebSharedWorkerImpl and WebHelperPluginImpl, as long as the "1
RenderFrameImpl per Frame" is stuck behind a flag, I can't move the
WebFrame identifier out of WebFrame quite yet.

-Albert


On Thu, Sep 12, 2013 at 9:32 PM, Darin Fisher <darin@chromium.org> wrote:

> I think the frame identifier is used by the Chrome Extension system in its
> WebRequest API.
>
> -Darin
>
>
> On Thu, Sep 12, 2013 at 9:32 PM, Darin Fisher <darin@chromium.org> wrote:
>
>> I'd need to study WebSharedWorkerImpl.cpp and WebHelperPluginImpl.cpp
>> some more. The frame identifier is unlikely to matter for either, but I'd
>> need to confirm. I'm not so familiar with the latter.
>>
>> -Darin
>>
>>
>> On Thu, Sep 12, 2013 at 9:25 PM, <ajwong@chromium.org> wrote:
>>
>>> Thanks for the comments!  I'll address most of them tomorrow. However, I
>>> wanted
>>> to respond to the WebFrame identifier piece.
>>>
>>>
>>>
>>> https://codereview.chromium.**org/23506013/diff/21001/**
>>>
Source/web/WebFrameImpl.cpp<https://codereview.chromium.org/23506013/diff/21001/Source/web/WebFrameImpl.cpp>
>>> File Source/web/WebFrameImpl.cpp (right):
>>>
>>> https://codereview.chromium.**org/23506013/diff/21001/**
>>>
Source/web/WebFrameImpl.cpp#**newcode2118<https://codereview.chromium.org/23506013/diff/21001/Source/web/WebFrameImpl.cpp#newcode2118>
>>> Source/web/WebFrameImpl.cpp:**2118: , m_identifier(identifier)
>>> On 2013/09/13 04:02:47, darin wrote:
>>>
>>>> We should just move the concept of WebFrame identifier to Chromium. it
>>>>
>>> doesn't
>>>
>>>> need to be part of Blink at all.  it was a mistake that it was ever
>>>>
>>> put here.
>>>
>>>> this identifier can be a property of the content::RenderFrameImpl on
>>>>
>>> the
>>>
>>>> Chromium side.
>>>>
>>>
>>> I did this initially, but didn't know how to handle
>>> WebSharedWorkerImpl.cpp and WebHelperPluginImpl.cpp. Both these create a
>>> WebFrameImpl outside the purview of RenderFrameImpl.
>>>
>>> WebHelplerPluginImpl in particular uses the mainframe's
>>> render_frame_impl as the client.  I have no idea whether or not
>>> WebHelperPluginImpl executes resource load requests.  If it does, then I
>>> think it needs to have a unique WebFrame identifier which means I can't
>>> move the WebFrame identifier into render_view just yet.
>>>
>>> Do you know whether it'd be okay for WebHelperPluginImpl to not have a
>>> sensible WebFrame identifier?
>>>
>>> Further more, since the per-frame RenderFrameHostImpl creation is
>>> sitting behind a flag, I can't make this transition yet.  Otherwise, I
>>> think I would break resource loads on trunk. :'(
>>>
>>>
https://codereview.chromium.**org/23506013/<https://codereview.chromium.org/2...
>>>
>>
>>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[darin (slow to review), 2013-09-13 04:52:31]

Good point. Perhaps just add a FIXME noting this temporary wart.
On Sep 12, 2013 9:46 PM, "Albert J. Wong (王重傑)" <ajwong@chromium.org> wrote:

> I'll look at it too, but even if it's okay to ignore them for
> WebSharedWorkerImpl and WebHelperPluginImpl, as long as the "1
> RenderFrameImpl per Frame" is stuck behind a flag, I can't move the
> WebFrame identifier out of WebFrame quite yet.
>
> -Albert
>
>
> On Thu, Sep 12, 2013 at 9:32 PM, Darin Fisher <darin@chromium.org> wrote:
>
>> I think the frame identifier is used by the Chrome Extension system in
>> its WebRequest API.
>>
>> -Darin
>>
>>
>> On Thu, Sep 12, 2013 at 9:32 PM, Darin Fisher <darin@chromium.org> wrote:
>>
>>> I'd need to study WebSharedWorkerImpl.cpp and WebHelperPluginImpl.cpp
>>> some more. The frame identifier is unlikely to matter for either, but I'd
>>> need to confirm. I'm not so familiar with the latter.
>>>
>>> -Darin
>>>
>>>
>>> On Thu, Sep 12, 2013 at 9:25 PM, <ajwong@chromium.org> wrote:
>>>
>>>> Thanks for the comments!  I'll address most of them tomorrow. However,
>>>> I wanted
>>>> to respond to the WebFrame identifier piece.
>>>>
>>>>
>>>>
>>>> https://codereview.chromium.**org/23506013/diff/21001/**
>>>>
Source/web/WebFrameImpl.cpp<https://codereview.chromium.org/23506013/diff/21001/Source/web/WebFrameImpl.cpp>
>>>> File Source/web/WebFrameImpl.cpp (right):
>>>>
>>>> https://codereview.chromium.**org/23506013/diff/21001/**
>>>>
Source/web/WebFrameImpl.cpp#**newcode2118<https://codereview.chromium.org/23506013/diff/21001/Source/web/WebFrameImpl.cpp#newcode2118>
>>>> Source/web/WebFrameImpl.cpp:**2118: , m_identifier(identifier)
>>>> On 2013/09/13 04:02:47, darin wrote:
>>>>
>>>>> We should just move the concept of WebFrame identifier to Chromium. it
>>>>>
>>>> doesn't
>>>>
>>>>> need to be part of Blink at all.  it was a mistake that it was ever
>>>>>
>>>> put here.
>>>>
>>>>> this identifier can be a property of the content::RenderFrameImpl on
>>>>>
>>>> the
>>>>
>>>>> Chromium side.
>>>>>
>>>>
>>>> I did this initially, but didn't know how to handle
>>>> WebSharedWorkerImpl.cpp and WebHelperPluginImpl.cpp. Both these create a
>>>> WebFrameImpl outside the purview of RenderFrameImpl.
>>>>
>>>> WebHelplerPluginImpl in particular uses the mainframe's
>>>> render_frame_impl as the client.  I have no idea whether or not
>>>> WebHelperPluginImpl executes resource load requests.  If it does, then I
>>>> think it needs to have a unique WebFrame identifier which means I can't
>>>> move the WebFrame identifier into render_view just yet.
>>>>
>>>> Do you know whether it'd be okay for WebHelperPluginImpl to not have a
>>>> sensible WebFrame identifier?
>>>>
>>>> Further more, since the per-frame RenderFrameHostImpl creation is
>>>> sitting behind a flag, I can't make this transition yet.  Otherwise, I
>>>> think I would break resource loads on trunk. :'(
>>>>
>>>>
https://codereview.chromium.**org/23506013/<https://codereview.chromium.org/2...
>>>>
>>>
>>>
>>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[awong, 2013-09-18 22:04:47]

Ready for a real review!

https://codereview.chromium.org/23506013/diff/21001/Source/web/WebHelperPlugi...
File Source/web/WebHelperPluginImpl.cpp (right):

https://codereview.chromium.org/23506013/diff/21001/Source/web/WebHelperPlugi...
Source/web/WebHelperPluginImpl.cpp:153: m_mainFrame =
static_cast<WebFrameImpl*>(WebFrame::create(client));
On 2013/09/13 04:02:47, darin wrote:
> perhaps this guy should just use WebFrameImpl::create ?

Done.

https://codereview.chromium.org/23506013/diff/21001/Source/web/tests/FrameTes...
File Source/web/tests/FrameTestHelpers.cpp (right):

https://codereview.chromium.org/23506013/diff/21001/Source/web/tests/FrameTes...
Source/web/tests/FrameTestHelpers.cpp:115: m_mainFrame =
WebFrame::create(webFrameClient);
On 2013/09/13 04:02:47, darin wrote:
> nit: allocate m_mainFrame closer to where it is first used?

Done.

https://codereview.chromium.org/23506013/diff/21001/public/web/WebView.h
File public/web/WebView.h (right):

https://codereview.chromium.org/23506013/diff/21001/public/web/WebView.h#newc...
public/web/WebView.h:101: virtual void initializeMainFrame(WebFrame*) = 0;
On 2013/09/13 04:02:47, darin wrote:
> Perhaps this method should be renamed to setMainFrame? The comments should
> probably be updated accordingly too.

I added a second method.  This is necessary to facilitate landing.  I'm unsure
what to update on the comment though. It seems accurate to me.

[darin (slow to review), 2013-09-19 22:36:18]

https://codereview.chromium.org/23506013/diff/58001/Source/web/WebFrameImpl.cpp
File Source/web/WebFrameImpl.cpp (right):

https://codereview.chromium.org/23506013/diff/58001/Source/web/WebFrameImpl.c...
Source/web/WebFrameImpl.cpp:559: return embedderIdentifier();
maybe identifier() should just be implemented as an inline function?

https://codereview.chromium.org/23506013/diff/58001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/23506013/diff/58001/Source/web/WebViewImpl.cp...
Source/web/WebViewImpl.cpp:324: // NOTE: The WebFrameImpl takes a reference to
itself within InitMainFrame
nit: InitMainFrame -> initializeAsMainFrame.

https://codereview.chromium.org/23506013/diff/58001/public/web/WebFrame.h
File public/web/WebFrame.h (right):

https://codereview.chromium.org/23506013/diff/58001/public/web/WebFrame.h#new...
public/web/WebFrame.h:96: // Creates a WebFrame with a self references. Call
close() to release
nit: "a self references"

Come to think of it, I don't understand the point of mentioning the internal
reference counting here. From the point of view of the embedder, WebFrame is not
reference counted. It is allocated using WebFrame::create() and destroyed using
WebFrame::close().

https://codereview.chromium.org/23506013/diff/58001/public/web/WebFrame.h#new...
public/web/WebFrame.h:97: // this reference. It is valid to pass null client
pointers.
There is only one client pointer, so "null client pointers" -> "a null client
pointer".

https://codereview.chromium.org/23506013/diff/58001/public/web/WebFrame.h#new...
public/web/WebFrame.h:105: // FIXME: Move the embedder_identifier concept fully
to the embedder and
nit: embedder_identifier -> embedderIdentifier

How about just overloading the one create method with an optional
embedderIdentifier? Use -1 as the default value so the implementation can know
to allocate the identifier?

https://codereview.chromium.org/23506013/diff/58001/public/web/WebFrame.h#new...
public/web/WebFrame.h:130: 
nit: one new line above, two below

https://codereview.chromium.org/23506013/diff/58001/public/web/WebFrame.h#new...
public/web/WebFrame.h:150: virtual long long embedderIdentifier() const = 0;
is this renaming just to make it easier to grep the code base for usage?

https://codereview.chromium.org/23506013/diff/58001/public/web/WebFrameClient.h
File public/web/WebFrameClient.h (right):

https://codereview.chromium.org/23506013/diff/58001/public/web/WebFrameClient...
public/web/WebFrameClient.h:115: // frameDetached() is called on it.
Isn't the WebFrame valid until the embedder calls close() on it?

[nasko, 2013-09-19 22:54:12]

Just a few clarifying comments.

https://codereview.chromium.org/23506013/diff/58001/Source/web/tests/FrameTes...
File Source/web/tests/FrameTestHelpers.cpp (right):

https://codereview.chromium.org/23506013/diff/58001/Source/web/tests/FrameTes...
Source/web/tests/FrameTestHelpers.cpp:131: reset();
Unneeded reset? The subsequent call to initialize will reset it as the first
order of business.

https://codereview.chromium.org/23506013/diff/58001/Source/web/tests/FrameTes...
Source/web/tests/FrameTestHelpers.cpp:147: if (m_mainFrame) {
Shouldn't you close the main frame before destroying the WebView?

https://codereview.chromium.org/23506013/diff/58001/Source/web/tests/LinkHigh...
File Source/web/tests/LinkHighlightTest.cpp (right):

https://codereview.chromium.org/23506013/diff/58001/Source/web/tests/LinkHigh...
Source/web/tests/LinkHighlightTest.cpp:196: webViewHelper.reset();
Unneeded reset?

https://codereview.chromium.org/23506013/diff/58001/Source/web/tests/WebViewT...
File Source/web/tests/WebViewTest.cpp (right):

https://codereview.chromium.org/23506013/diff/58001/Source/web/tests/WebViewT...
Source/web/tests/WebViewTest.cpp:356: m_webViewHelper.reset(); // Remove
dependency on locally scoped client.
Why is this test different than the rest in requiring an explicit reset? Maybe
put a comment to explain it?
There are a few more occurrences in this file, so I won't flag them all.

https://codereview.chromium.org/23506013/diff/58001/public/web/WebFrame.h
File public/web/WebFrame.h (right):

https://codereview.chromium.org/23506013/diff/58001/public/web/WebFrame.h#new...
public/web/WebFrame.h:96: // Creates a WebFrame with a self references. Call
close() to release
nit: a reference or references

https://codereview.chromium.org/23506013/diff/58001/public/web/WebFrame.h#new...
public/web/WebFrame.h:105: // FIXME: Move the embedder_identifier concept fully
to the embedder and
nit: Is FIXME the preferred way for Blink? Not sure, just asking.

https://codereview.chromium.org/23506013/diff/58001/public/web/WebFrame.h#new...
public/web/WebFrame.h:131: // This method closes and deletes the WebFrame.
In the comment for create() you say that close will release the reference, but
here you add that it will also delete. Should the two match?

[awong, 2013-09-20 00:36:23]

Comments addressed.

https://codereview.chromium.org/23506013/diff/58001/Source/web/WebFrameImpl.cpp
File Source/web/WebFrameImpl.cpp (right):

https://codereview.chromium.org/23506013/diff/58001/Source/web/WebFrameImpl.c...
Source/web/WebFrameImpl.cpp:559: return embedderIdentifier();
On 2013/09/19 22:36:19, darin wrote:
> maybe identifier() should just be implemented as an inline function?

sure. de-virtualized and put into WebFrame.h.

https://codereview.chromium.org/23506013/diff/58001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/23506013/diff/58001/Source/web/WebViewImpl.cp...
Source/web/WebViewImpl.cpp:324: // NOTE: The WebFrameImpl takes a reference to
itself within InitMainFrame
On 2013/09/19 22:36:19, darin wrote:
> nit: InitMainFrame -> initializeAsMainFrame.

Done.

https://codereview.chromium.org/23506013/diff/58001/Source/web/tests/FrameTes...
File Source/web/tests/FrameTestHelpers.cpp (right):

https://codereview.chromium.org/23506013/diff/58001/Source/web/tests/FrameTes...
Source/web/tests/FrameTestHelpers.cpp:131: reset();
On 2013/09/19 22:54:12, nasko wrote:
> Unneeded reset? The subsequent call to initialize will reset it as the first
> order of business.

Done.

https://codereview.chromium.org/23506013/diff/58001/Source/web/tests/FrameTes...
Source/web/tests/FrameTestHelpers.cpp:147: if (m_mainFrame) {
On 2013/09/19 22:54:12, nasko wrote:
> Shouldn't you close the main frame before destroying the WebView?

Nope...the webview expects the mainframe to be alive until it singles its own
destruction.

https://codereview.chromium.org/23506013/diff/58001/Source/web/tests/LinkHigh...
File Source/web/tests/LinkHighlightTest.cpp (right):

https://codereview.chromium.org/23506013/diff/58001/Source/web/tests/LinkHigh...
Source/web/tests/LinkHighlightTest.cpp:196: webViewHelper.reset();
On 2013/09/19 22:54:12, nasko wrote:
> Unneeded reset?

Done.

https://codereview.chromium.org/23506013/diff/58001/Source/web/tests/WebViewT...
File Source/web/tests/WebViewTest.cpp (right):

https://codereview.chromium.org/23506013/diff/58001/Source/web/tests/WebViewT...
Source/web/tests/WebViewTest.cpp:356: m_webViewHelper.reset(); // Remove
dependency on locally scoped client.
On 2013/09/19 22:54:12, nasko wrote:
> Why is this test different than the rest in requiring an explicit reset? Maybe
> put a comment to explain it?
> There are a few more occurrences in this file, so I won't flag them all.

I tried to clarify the existing comment a little.

https://codereview.chromium.org/23506013/diff/58001/public/web/WebFrame.h
File public/web/WebFrame.h (right):

https://codereview.chromium.org/23506013/diff/58001/public/web/WebFrame.h#new...
public/web/WebFrame.h:96: // Creates a WebFrame with a self references. Call
close() to release
On 2013/09/19 22:36:19, darin wrote:
> nit: "a self references"
> 
> Come to think of it, I don't understand the point of mentioning the internal
> reference counting here. From the point of view of the embedder, WebFrame is
not
> reference counted. It is allocated using WebFrame::create() and destroyed
using
> WebFrame::close().

Remove discussion of refcounting.

https://codereview.chromium.org/23506013/diff/58001/public/web/WebFrame.h#new...
public/web/WebFrame.h:96: // Creates a WebFrame with a self references. Call
close() to release
On 2013/09/19 22:54:12, nasko wrote:
> nit: a reference or references

Rewrote comment.

https://codereview.chromium.org/23506013/diff/58001/public/web/WebFrame.h#new...
public/web/WebFrame.h:97: // this reference. It is valid to pass null client
pointers.
On 2013/09/19 22:36:19, darin wrote:
> There is only one client pointer, so "null client pointers" -> "a null client
> pointer".

Fix here and also for WebView::create().

https://codereview.chromium.org/23506013/diff/58001/public/web/WebFrame.h#new...
public/web/WebFrame.h:105: // FIXME: Move the embedder_identifier concept fully
to the embedder and
On 2013/09/19 22:36:19, darin wrote:
> nit: embedder_identifier -> embedderIdentifier
> 
> How about just overloading the one create method with an optional
> embedderIdentifier? Use -1 as the default value so the implementation can know
> to allocate the identifier?

Is there a benefit to using a default arguments?  It barely saves code in this
case.  I'm conditioned to avoid default arguments.

https://codereview.chromium.org/23506013/diff/58001/public/web/WebFrame.h#new...
public/web/WebFrame.h:130: 
On 2013/09/19 22:36:19, darin wrote:
> nit: one new line above, two below

Done.

https://codereview.chromium.org/23506013/diff/58001/public/web/WebFrame.h#new...
public/web/WebFrame.h:131: // This method closes and deletes the WebFrame.
On 2013/09/19 22:54:12, nasko wrote:
> In the comment for create() you say that close will release the reference, but
> here you add that it will also delete. Should the two match?

Done.

https://codereview.chromium.org/23506013/diff/58001/public/web/WebFrame.h#new...
public/web/WebFrame.h:131: // This method closes and deletes the WebFrame.
On 2013/09/19 22:54:12, nasko wrote:
> In the comment for create() you say that close will release the reference, but
> here you add that it will also delete. Should the two match?

Done.

https://codereview.chromium.org/23506013/diff/58001/public/web/WebFrame.h#new...
public/web/WebFrame.h:150: virtual long long embedderIdentifier() const = 0;
On 2013/09/19 22:36:19, darin wrote:
> is this renaming just to make it easier to grep the code base for usage?

Nope. The renaming is because this same class has another concept actually
called "identifier" with type "int" instead of long long.  See find(),
scopeStringMatches(), and increaseMatchCount().  That confused the heck out of
me for a while.

https://codereview.chromium.org/23506013/diff/58001/public/web/WebFrameClient.h
File public/web/WebFrameClient.h (right):

https://codereview.chromium.org/23506013/diff/58001/public/web/WebFrameClient...
public/web/WebFrameClient.h:115: // frameDetached() is called on it.
On 2013/09/19 22:36:19, darin wrote:
> Isn't the WebFrame valid until the embedder calls close() on it?

Hmm...would "in-use" be a better term?

I'm trying to say that the client implementer should expect the returned
WebFrame can be deleted when frameDetached() is called.

[dcheng, 2013-09-20 02:29:59]

https://codereview.chromium.org/23506013/diff/90001/Source/web/WebFrameImpl.cpp
File Source/web/WebFrameImpl.cpp (right):

https://codereview.chromium.org/23506013/diff/90001/Source/web/WebFrameImpl.c...
Source/web/WebFrameImpl.cpp:2192: if (mustCallDidCreateFrame && m_client)
Just curious--why do we need to verify m_client is non-NULL here but we don't
care on previous lines? Ditto for below.

https://codereview.chromium.org/23506013/diff/90001/Source/web/tests/FrameTes...
File Source/web/tests/FrameTestHelpers.cpp (right):

https://codereview.chromium.org/23506013/diff/90001/Source/web/tests/FrameTes...
Source/web/tests/FrameTestHelpers.cpp:61: class TestWebFrameClient : public
WebFrameClient {
Any reason we can't just use WebFrameClient and WebViewClient directly?

https://codereview.chromium.org/23506013/diff/90001/Source/web/tests/FrameTes...
File Source/web/tests/FrameTestHelpers.h (right):

https://codereview.chromium.org/23506013/diff/90001/Source/web/tests/FrameTes...
Source/web/tests/FrameTestHelpers.h:36: #include <string>
Hm... should #include <string> be at the top of the includes?

https://codereview.chromium.org/23506013/diff/90001/Source/web/tests/FrameTes...
Source/web/tests/FrameTestHelpers.h:51: class WebViewHelper {
WTF_MAKE_NONCOPYABLE?

[awong, 2013-09-20 23:57:13]

PTAL

https://codereview.chromium.org/23506013/diff/90001/Source/web/WebFrameImpl.cpp
File Source/web/WebFrameImpl.cpp (right):

https://codereview.chromium.org/23506013/diff/90001/Source/web/WebFrameImpl.c...
Source/web/WebFrameImpl.cpp:2192: if (mustCallDidCreateFrame && m_client)
On 2013/09/20 02:29:59, dcheng wrote:
> Just curious--why do we need to verify m_client is non-NULL here but we don't
> care on previous lines? Ditto for below.

You're right. That's vestigial. Fixed.

https://codereview.chromium.org/23506013/diff/90001/Source/web/tests/FrameTes...
File Source/web/tests/FrameTestHelpers.cpp (right):

https://codereview.chromium.org/23506013/diff/90001/Source/web/tests/FrameTes...
Source/web/tests/FrameTestHelpers.cpp:61: class TestWebFrameClient : public
WebFrameClient {
On 2013/09/20 02:29:59, dcheng wrote:
> Any reason we can't just use WebFrameClient and WebViewClient directly?

Good point. Was just cut and paste, but it does seem silly. Fixed.

https://codereview.chromium.org/23506013/diff/90001/Source/web/tests/FrameTes...
File Source/web/tests/FrameTestHelpers.h (right):

https://codereview.chromium.org/23506013/diff/90001/Source/web/tests/FrameTes...
Source/web/tests/FrameTestHelpers.h:36: #include <string>
On 2013/09/20 02:29:59, dcheng wrote:
> Hm... should #include <string> be at the top of the includes?

I have no clue. This is what passed the blink style checks. I can move it back
up, and the --bypass-hooks on upload.

https://codereview.chromium.org/23506013/diff/90001/Source/web/tests/FrameTes...
Source/web/tests/FrameTestHelpers.h:51: class WebViewHelper {
On 2013/09/20 02:29:59, dcheng wrote:
> WTF_MAKE_NONCOPYABLE?

Done.

[darin (slow to review), 2013-09-21 04:04:17]

LGTM

[commit-bot: I haz the power, 2013-09-21 04:16:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ajwong@chromium.org/23506013/96001

[awong, 2013-09-25 23:24:56]

FYI, I found a bug where I introduced an extra m_client->frameDetach() during
the error case inside WebFrameImpl::createChildFrame().  I had thought that if
we have a synchronous detach of the current (aka parent) frame, we would need to
manually call frameDetach() on the child frame.  This was a mistake. The
FrameLoaderClient already does this.

I've removed the extra frameDetach() and checked in a debugger that the
constructor/destructor calls for WebFrameImpl match up.

[commit-bot: I haz the power, 2013-09-25 23:25:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ajwong@chromium.org/23506013/116001

[commit-bot: I haz the power, 2013-09-26 06:01:49]

Commit queue rejected this change because the description was changed
between the time the change entered the commit queue and the time it
was ready to commit. You can safely check the commit box again.

[commit-bot: I haz the power, 2013-09-26 06:21:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ajwong@chromium.org/23506013/116001

[commit-bot: I haz the power, 2013-09-26 06:22:58]

Change committed as 158354