Make the embedder responsible for creating the WebFrame

public/web/WebFrame.h

Index: public/web/WebFrame.h
diff --git a/public/web/WebFrame.h b/public/web/WebFrame.h
index 211def68aed06bc968232765d3d6b6d4962f7ed1..0847989310df544c6980253f1bf2dc330fa3da50 100644
--- a/public/web/WebFrame.h
+++ b/public/web/WebFrame.h
@@ -59,6 +59,7 @@ class WebDataSource;
 class WebDocument;
 class WebElement;
 class WebFormElement;
+class WebFrameClient;
 class WebHistoryItem;
 class WebInputElement;
 class WebPerformance;
@@ -92,6 +93,23 @@ public:
     };
     typedef unsigned RenderAsTextControls;
 
+    // Creates a WebFrame with a self references. Call close() to release

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

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().

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

nit: a reference or references

[awong, 2013/09/20 00:36:24]

Remove discussion of refcounting.

[awong, 2013/09/20 00:36:24]

Rewrote comment.

+    // this reference. It is valid to pass null client pointers.

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

There is only one client pointer, so "null client pointers" -> "a null client
pointer".

[awong, 2013/09/20 00:36:24]

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

+    WEBKIT_EXPORT static WebFrame* create(WebFrameClient*);
+
+    // Same as create(WebFrameClient*) except the embedder may explicitly pass
+    // in the identifier for the WebFrame. This can be used with
+    // generateEmbedderIdentifier() if constructing the WebFrameClient for this
+    // frame requires the identifier.
+    //
+    // FIXME: Move the embedder_identifier concept fully to the embedder and

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

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?

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

nit: Is FIXME the preferred way for Blink? Not sure, just asking.

[awong, 2013/09/20 00:36:24]

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

+    // remove this factory method.
+    WEBKIT_EXPORT static WebFrame* create(WebFrameClient*, long long embedderIdentifier);
+
+    // Generates an identifier suitable for use with create() above.
+    // Never returns -1.
+    WEBKIT_EXPORT static long long generateEmbedderIdentifier();
+
     // Returns the number of live WebFrame objects, used for leak checking.
     WEBKIT_EXPORT static int instanceCount();
 
@@ -110,6 +128,9 @@ public:
     WEBKIT_EXPORT static WebFrame* fromFrameOwnerElement(const WebElement&);
 
 

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

nit: one new line above, two below

[awong, 2013/09/20 00:36:24]

Done.

+    // This method closes and deletes the WebFrame.

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

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:24]

Done.

[awong, 2013/09/20 00:36:24]

Done.

+    virtual void close() = 0;
+
     // Basic properties ---------------------------------------------------
 
     // The unique name of this frame.
@@ -124,7 +145,9 @@ public:
     virtual void setName(const WebString&) = 0;
 
     // A globally unique identifier for this frame.
+    // FIXME: Convert users to embedderIdentifier() and remove identifier().
     virtual long long identifier() const = 0;
+    virtual long long embedderIdentifier() const = 0;

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

is this renaming just to make it easier to grep the code base for usage?

[awong, 2013/09/20 00:36:24]

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.

 
     // The urls of the given combination types of favicon (if any) specified by
     // the document loaded in this frame. The iconTypesMask is a bit-mask of