Make the embedder responsible for creating the WebFrame

public/web/WebFrame.h

 /*
  * Copyright (C) 2009 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
  *     * Redistributions in binary form must reproduce the above
@@ skipping 41 matching lines @@
 template <class T> class Local;
 }
 
 namespace WebKit {
 
 class WebData;
 class WebDataSource;
 class WebDocument;
 class WebElement;
 class WebFormElement;
+class WebFrameClient;
 class WebHistoryItem;
 class WebInputElement;
 class WebPerformance;
 class WebRange;
 class WebSecurityOrigin;
 class WebString;
 class WebURL;
 class WebURLLoader;
 class WebURLRequest;
 class WebView;
@@ skipping 13 matching lines @@
 class WebFrame {
 public:
     // Control of renderTreeAsText output
     enum RenderAsTextControl {
         RenderAsTextNormal = 0,
         RenderAsTextDebug = 1 << 0,
         RenderAsTextPrinting = 1 << 1
     };
     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();
 
     // Returns the WebFrame associated with the current V8 context. This
     // function can return 0 if the context is associated with a Document that
     // is not currently being displayed in a Frame.
     WEBKIT_EXPORT static WebFrame* frameForCurrentContext();
 
     // Returns the frame corresponding to the given context. This can return 0
     // if the context is detached from the frame, or if the context doesn't
     // correspond to a frame (e.g., workers).
     WEBKIT_EXPORT static WebFrame* frameForContext(v8::Handle<v8::Context>);
 
     // Returns the frame inside a given frame or iframe element. Returns 0 if
     // the given element is not a frame, iframe or if the frame is empty.
     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.
     virtual WebString uniqueName() const = 0;
 
     // The name of this frame. If no name is given, empty string is returned.
     virtual WebString assignedName() const = 0;
 
     // Sets the name of this frame. For child frames (frames that are not a
     // top-most frame) the actual name may have a suffix appended to make the
     // frame name unique within the hierarchy.
     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
     // WebIconURL::Type values, used to select from the available set of icon
     // URLs
     virtual WebVector<WebIconURL> iconURLs(int iconTypesMask) const = 0;
 
 
     // Geometry -----------------------------------------------------------
 
@@ skipping 512 matching lines @@
     // text form. This is used only by layout tests.
     virtual WebString layerTreeAsText(bool showDebugInfo = false) const = 0;
 
 protected:
     ~WebFrame() { }
 };
 
 } // namespace WebKit
 
 #endif