Replicating WebFrame::uniqueName across renderers.

components/html_viewer/html_frame.cc

 // Copyright 2015 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.
 
 #include "components/html_viewer/html_frame.h"
 
 #include <stddef.h>
 #include <algorithm>
 #include <limits>
 #include <utility>
@@ skipping 115 matching lines @@
       weak_factory_(this) {
   TRACE_EVENT0("html_viewer", "HTMLFrame::HTMLFrame");
   if (parent_)
     parent_->children_.push_back(this);
 
   if (params->window && params->window->id() == id_)
     SetWindow(params->window);
 
   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();
 
     // This is the root of the tree (aka the main frame).
     // Expected order for creating webframes is:
     // . Create local webframe (first webframe must always be local).
     // . Set as main frame on WebView.
     // . Swap to remote (if not local).
     blink::WebLocalFrame* local_web_frame =
         blink::WebLocalFrame::create(state_.tree_scope, this);
@@ skipping 34 matching lines @@
           StatsCollectionController::Install(web_frame_, GetApp());
     }
   } else if (!params->is_local_create_child && params->window &&
              id_ == params->window->id()) {
     // Frame represents the local frame, and it isn't the root of the tree.
     HTMLFrame* previous_sibling = GetPreviousSibling(this);
     blink::WebFrame* previous_web_frame =
         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);
 
     blink::WebLocalFrame* child_web_frame =
         blink::WebLocalFrame::create(state_.tree_scope, this);
     web_frame_ = child_web_frame;
     parent_->web_frame_->appendChild(child_web_frame);
   }
 
   DVLOG(2) << "HTMLFrame init this=" << this << " id=" << id_
@@ skipping 111 matching lines @@
     const blink::WebString& sink_id,
     blink::WebMediaSession* media_session) {
   return global_state()->media_factory()->CreateMediaPlayer(
       frame, url, client, encrypted_client, initial_cdm, GetApp()->shell());
 }
 
 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;
   child_state.sandbox_flags = sandbox_flags;
   mojo::Map<mojo::String, mojo::Array<uint8_t>> client_properties;
   client_properties.mark_non_null();
   ClientPropertiesFromReplicatedFrameState(child_state, &client_properties);
 
   child_window->SetVisible(true);
   window_->AddChild(child_window);
@@ skipping 126 matching lines @@
   server_->LoadingStateChanged(true, load_progress);
 }
 
 void HTMLFrame::dispatchLoad() {
   // According to comments of WebFrameClient::dispatchLoad(), this should only
   // be called when the parent frame is remote.
   DCHECK(parent_ && !parent_->IsLocal());
   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));
 }
 
 void HTMLFrame::didCommitProvisionalLoad(
     blink::WebLocalFrame* frame,
     const blink::WebHistoryItem& item,
     blink::WebHistoryCommitType commit_type) {
   state_.origin = FrameOrigin(frame);
@@ skipping 546 matching lines @@
 
   if (!surface_layer_)
     return;
 
   surface_layer_->SetSurfaceId(cc::SurfaceId(owned_window_->window()->id()),
                                global_state()->device_pixel_ratio(),
                                owned_window_->window()->bounds().size());
 }
 
 }  // namespace mojo