Cross-process postMessage

content/browser/content_frame.h

+// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CONTENT_BROWSER_CONTENT_FRAME_H_
+#define CONTENT_BROWSER_CONTENT_FRAME_H_
+
+#include "base/basictypes.h"
+#include "base/hash_tables.h"
+#include "content/browser/tab_contents/tab_contents.h"
+#include "content/browser/tab_contents/tab_contents_observer.h"
+#include <list>

[awong, 2011/12/21 01:06:38]

C++ includes go first. Separate by a newline.

[supersat, 2011/12/23 03:22:46]

Done.

+
+namespace content {

[awong, 2011/12/21 01:06:38]

newline after namespace opener.

[supersat, 2011/12/23 03:22:46]

Done.

+class FrameMap;
+
+// This class represents a single user-visible frame and tracks all of the
+// (potentially invisible) WebKit frames that may either be currently rendering
+// the frame or are proxies for the frame. In conjunction with the FrameMap,
+// this is used to route cross-process scripting calls from a proxy WebKit frame
+// to the WebKit frame that's currently active.

[awong, 2011/12/21 01:06:38]

For single class files, It's generally preferable to stuff this as the file
comment.

[jam, 2011/12/22 20:06:53]

really? i didn't know that.

the style is to actually put one class per file. old-style was to put multiple
classes/structs in a single file, which we now discourage. i find it personally
more readable to have the class comment next to the class itself, as opposed to
separated by all the headers, forward declares etc

[awong, 2011/12/22 20:09:04]

Yes...Karl and I talked about this and I was mistaken.

[supersat, 2011/12/23 03:22:46]

Discussed offline. Agreed that keeping the comment here is most consistent.

+class ContentFrame {

[jam, 2011/12/22 20:06:53]

I'm curious why you picked ContentFrame? I ask because it doesn't match the
existing name for process/pages
(RenderProcessHost/RenderViewHost/RenderWidgetHost) and hence RenderFrameHost
would be more consistent?

[Charlie Reis, 2011/12/22 20:13:21]

Wouldn't that imply that there's a RenderFrame it's communicating with in the
renderer process?  Actually, I suppose there's a WebFrame that this corresponds
to.  Should it be called WebFrameHost?

+ friend class FrameMap;

[awong, 2011/12/21 01:06:38]

Ick. friends. I don't like friends. That's probably why I don't have any.

Can we get rid of your friends too? ;)

[supersat, 2011/12/23 03:22:46]

Everyone seems to be hating on friends, so I'll remove it. The idea was to
prevent anyone but the FrameMap from updating the ContentFrame, since the maps
in FrameMap need to stay in sync with the ContentFrame.

+
+ public:
+  // TODO(supersat): Comment this once I figure out exactly what it'll contain

[awong, 2011/12/21 01:06:38]

Can we pull this out into the content namespace?  Nested classes are annoying. 
Since this is an identifier that is used external to this class, it feels like
it should be de-nested.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Nested...

[jam, 2011/12/22 20:06:53]

nit: we try to avoid nested classes per the style guide, since then if anyone
else wants to refer to the struct in a header they have to include this whole
header. agree that putting it into content namespace is better, but also in a
separate file

[supersat, 2011/12/23 03:22:46]

Done.

+  struct WebKitFrameIdTuple {

[awong, 2011/12/21 01:06:38]

Assuming this doesn't get fragmented/renamed into smaller structs, I would drop
the "Tuple" from the name. It doesn't add more any more description to the name,
and "struct" is more of a record type than a tuple since the members are
label-based rather than positional.

[jam, 2011/12/22 20:06:53]

+1

[supersat, 2011/12/23 03:22:46]

Done.

+    WebKitFrameIdTuple(int process_host_id, int route_id, int64 frame_id);
+
+    // The == operator does NOT compare the route_id!

[awong, 2011/12/21 01:06:38]

Is it possible to pull route_id out of this struct then?

It sounds like you have the identifying pair (process_host_id, frame_id), and
then you have the data it maps to (route_id).  This struct seems to comingle the
two ideas.

+    bool operator==(const WebKitFrameIdTuple& other) const;

[awong, 2011/12/21 01:06:38]

Chromium style does not allow operator overloading.  Use an external comparator
instead?

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Operat...

+
+    int process_host_id;
+    int route_id;
+    int64 frame_id;

[jam, 2011/12/22 20:06:53]

why use 64 bit numbers for frame id? 32bit is fine for route_ids, and i really
doubt we'll ever wrap around

[supersat, 2011/12/23 03:22:46]

WebFrame's identifier is 64-bit. I'm not sure why it's 64-bit (it was this way
before WebFrame was upstreamed into WebKit), but it is.

+  };
+
+  ContentFrame(int64 id,
+               bool is_top_level,
+               TabContents& tab_contents,
+               ContentFrame* opener);
+  virtual ~ContentFrame();

[awong, 2011/12/21 01:06:38]

No methods are virtual.  Why is the destructor virtual?

[supersat, 2011/12/23 03:22:46]

Done.

+
+  // Returns the frame's globally-unique identifier.
+  int64 id() { return id_; }
+
+  // Returns the WebKitFrameIdTuple that uniquely identifies the WebKit frame
+  // that is actively rendering this frame.
+  const WebKitFrameIdTuple current_webkit_frame() const {

[awong, 2011/12/21 01:06:38]

What do you think about "active" instead of "current" as variable/API name? 
That's the adjective you're using in the comments, and it seems more
descriptive/less "context dependent" than "current."

[supersat, 2011/12/23 03:22:46]

Done.

+    return current_webkit_frame_;
+  }
+
+  // Returns the process id for the WebKit frame that's actively rendering this
+  // frame.

[awong, 2011/12/21 01:06:38]

These are duplicate accessors for data that can be retrieved via
current_webkit_frame().  Remove one API?

Having 2 mechanisms to access the same data make it harder to trace down state
dependencies for future maintainers.

[supersat, 2011/12/23 03:22:46]

Done.

+  int current_process_host_id() const {
+    return current_webkit_frame_.process_host_id;
+  }
+
+  // Returns the RenderView route id for the WebKit frame that's actively
+  // rendering this frame.
+  int current_route_id() const {
+    return current_webkit_frame_.route_id;
+  }
+
+  // Returns the WebKit frame id for the WebKit frame that's actively rendering
+  // this frame. Note that the WebKit frame id is only unique per-process!

[awong, 2011/12/21 01:06:38]

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Nested...


The first comment seems pretty obvious from the function name.  I would bias
towards describing what these are in the class comment and then letting the
accessors just stay uncommented unless you have a non-obvious caveat.

+  int64 current_frame_id() const {
+    return current_webkit_frame_.frame_id;
+  }
+
+  // Returns whether this frame is a top-level frame or not. Each RenderView and
+  // TabContents has one top-level frame.
+  bool is_top_level() const {
+    return is_top_level_;
+  }
+
+  // Returns the RenderViewHostDelegate for this frame.
+  TabContents& tab_contents() const {
+    return tab_contents_;
+  }
+
+  // Returns a list of all WebKit frames that are either rendering this frame or
+  // are proxies for this frame.
+  const std::list<WebKitFrameIdTuple> all_webkit_frames() const {

[awong, 2011/12/21 01:06:38]

const&?

+    return all_webkit_frames_;
+  }
+
+  // Updates which WebKit frame is currently rendering this frame.
+  void UpdateFrame(int new_process_host_id,

[awong, 2011/12/21 01:06:38]

How about SetActiveFrame() or ReplaceActiveFrame() or ChangeActiveFrame()?

+                   int new_route_id,
+                   int64 new_frame_id);
+
+  // Return this ContentFrame's opener.
+  ContentFrame* opener() const {
+    return opener_;
+  }
+
+ private:
+  class Observer : public TabContentsObserver {

[jam, 2011/12/22 20:06:53]

nit: this class is an internal implementation and doesn't need to be declared in
this header. usually we prefer to put these classes just in the cc file, to keep
the header minimal

+   public:
+    Observer(ContentFrame* frame);

[awong, 2011/12/21 01:06:38]

explicit.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Explic...

+    // TabContentsObserver methods
+    virtual void DidCommitProvisionalLoadForFrame(
+      int64 frame_id,
+      bool is_main_frame,
+      const GURL& url,
+      content::PageTransition transition_type) OVERRIDE;
+
+    virtual void TabContentsDestroyed(TabContents* tab) OVERRIDE;
+
+   private:
+    ContentFrame* frame_;
+  };
+
+  int64 id_;
+
+  WebKitFrameIdTuple current_webkit_frame_;
+  bool is_top_level_;
+
+  std::list<WebKitFrameIdTuple> all_webkit_frames_;
+
+  TabContents& tab_contents_;

[jam, 2011/12/22 20:06:53]

nit: i've never seen us storing references to TabContents before. usually a
pointer.

+
+  ContentFrame* opener_;
+};
+
+}  // namespace content
+
+#if defined(COMPILER_GCC)
+namespace __gnu_cxx {
+
+// Since the == operator does not compare the route_id, we don't include it in
+// the hash.
+template<>
+struct hash<content::ContentFrame::WebKitFrameIdTuple> {
+  std::size_t operator()(const

[awong, 2011/12/21 01:06:38]

bad wrap...

+      content::ContentFrame::WebKitFrameIdTuple& p) const {
+    return p.process_host_id * 257 + p.frame_id;

[awong, 2011/12/21 01:06:38]

Magic number! Explain?

+  }
+};
+
+}  // namespace __gnu_cxx
+#endif
+
+#endif  // CONTENT_BROWSER_CONTENT_FRAME_H_