Replicating WebFrame::uniqueName across renderers.

components/html_viewer/html_frame.cc

Index: components/html_viewer/html_frame.cc
diff --git a/components/html_viewer/html_frame.cc b/components/html_viewer/html_frame.cc
index 9cd02558e509a0752bfb304ed874baeb099e6565..2dd138eae85a287b1870d3c1ca273cd5dd617d0e 100644
--- a/components/html_viewer/html_frame.cc
+++ b/components/html_viewer/html_frame.cc
@@ -133,6 +133,9 @@ HTMLFrame::HTMLFrame(CreateParams* params)
 
   SetReplicatedFrameStateFromClientProperties(params->properties, &state_);
 
+  // TODO(lukasza): Add |frame_unique_name| to ReplicatedFrameState?
+  blink::WebString unique_name;  // Empty string = will recalculate in Blink.
+
   if (!parent_) {
     CreateRootWebWidget();
 
@@ -187,14 +190,15 @@ HTMLFrame::HTMLFrame(CreateParams* params)
         previous_sibling ? previous_sibling->web_frame() : nullptr;
     CHECK(!parent_->IsLocal());
     web_frame_ = parent_->web_frame()->toWebRemoteFrame()->createLocalChild(
-        state_.tree_scope, state_.name, state_.sandbox_flags, this,
+        state_.tree_scope, state_.name, unique_name, state_.sandbox_flags, this,
         previous_web_frame,
         // TODO(lazyboy): Replicate WebFrameOwnerProperties where needed.
         blink::WebFrameOwnerProperties());
     CreateLocalRootWebWidget(web_frame_->toWebLocalFrame());
   } else if (!parent_->IsLocal()) {
     web_frame_ = parent_->web_frame()->toWebRemoteFrame()->createRemoteChild(
-        state_.tree_scope, state_.name, state_.sandbox_flags, this);
+        state_.tree_scope, state_.name, unique_name, state_.sandbox_flags,
+        this);
   } else {
     CHECK(params->is_local_create_child);
 
@@ -326,14 +330,16 @@ blink::WebFrame* HTMLFrame::createChildFrame(
     blink::WebLocalFrame* parent,
     blink::WebTreeScopeType scope,
     const blink::WebString& frame_name,
+    const blink::WebString& /* frame_unique_name */,
     blink::WebSandboxFlags sandbox_flags,
-    const blink::WebFrameOwnerProperties& frame_owner_properties) {
+    const blink::WebFrameOwnerProperties& /* frame_owner_properties */) {
   DCHECK(IsLocal());  // Can't create children of remote frames.
   DCHECK_EQ(parent, web_frame_);
   DCHECK(window_);  // If we're local we have to have a window.
   // Create the window that will house the frame now. We embed once we know the
   // url (see decidePolicyForNavigation()).
   mus::Window* child_window = window_->connection()->NewWindow();
+  // TODO(lukasza): Add |frame_unique_name| to ReplicatedFrameState?
   ReplicatedFrameState child_state;
   child_state.name = frame_name;
   child_state.tree_scope = scope;
@@ -480,8 +486,10 @@ void HTMLFrame::dispatchLoad() {
   server_->DispatchLoadEventToParent();
 }
 
-void HTMLFrame::didChangeName(blink::WebLocalFrame* frame,
-                              const blink::WebString& name) {
+void HTMLFrame::didChangeName(blink::WebLocalFrame* /* frame */,

[alexmos, 2016/02/10 00:46:46]

Is this style ok?  I haven't really seen this (leaving arg names out) done
outside of Blink headers and IPC messages, and it doesn't seem like it's done
elsewhere in this file.

[Łukasz Anforowicz, 2016/02/10 22:10:50]

I don't remember where I first saw this style, but it is supposed to convey that
the given parameter is not used in the function/method body (i.e. looking at the
parameters list one can quickly see which parameters matter and which are
ignored).  This shouldn't matter anymore for html_frame.cc (because unrelated CL
removed this file so I don't need to make the change above), but I am open for
discussing this style in context of other files (I might have used similar style
elsewhere).

Charlie?  WDYT?

[Charlie Reis, 2016/02/11 22:02:14]

It's moot in this case, since this file is gone now, but it looks like it's used
in print_web_view_helper.cc as well.  In general, it's allowed by the style
guide but I thought it was discouraged in Chromium code.  I'd have to dig around
to find an old thread on it.

https://google.github.io/styleguide/cppguide.html#Function_Declarations_and_D...
"Parameter names may be omitted only if the parameter is unused and its purpose
is obvious."

Personally, I'd prefer to have the actual names there rather than comments,
since it's more consistent with other code around it and doesn't provide a huge
benefit.

[Łukasz Anforowicz, 2016/02/11 23:23:55]

Right, but 1) we are not "omitting" the parameter names here, we are only
commenting them out and 2) your link says a few lines below that "Unused
parameters that might not be obvious should comment out the variable name in the
function definition" and gives an example consistent with what I did above:

void Circle::Rotate(double /*radians*/) {}
Done.

+                              const blink::WebString& name,
+                              const blink::WebString& /* unique_name */) {
+  // TODO(lukasza): Add |frame_unique_name| to ReplicatedFrameState?
   state_.name = name;
   server_->SetClientProperty(kPropertyFrameName,
                              FrameNameToClientProperty(name));