Notify the change of brand color from renderer to Brower

content/renderer/render_frame_impl.cc

 // Copyright 2013 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 "content/renderer/render_frame_impl.h"
 
 #include <map>
 #include <string>
 
 #include "base/auto_reset.h"
@@ skipping 2187 matching lines @@
   didCommitProvisionalLoad(frame, item, commit_type);
 }
 
 void RenderFrameImpl::didUpdateCurrentHistoryItem(blink::WebLocalFrame* frame) {
   DCHECK(!frame_ || frame_ == frame);
   // TODO(nasko): Move implementation here. Needed methods:
   // * StartNavStateSyncTimerIfNecessary
   render_view_->didUpdateCurrentHistoryItem(frame);
 }
 
+void RenderFrameImpl::didChangeBrandColor(blink::WebLocalFrame* frame) {
+  DCHECK(!frame_ || frame_ == frame);

[dcheng, 2014/06/16 17:21:25]

Can frame_ ever be NULL here?

[michaelbai, 2014/06/16 17:49:22]

I don't know, but there is same code in the class, like
RenderFrameImpl::didChangeName.

On 2014/06/16 17:21:25, dcheng wrote:

[dcheng, 2014/06/16 18:52:19]

Right, but things like that get called on the initialization path for
WebLocalFrame/LocalFrame. I don't think this is hit on that path, so we should
remove the !frame_ check if we don't need it.

[jam, 2014/06/16 20:00:45]

this isn't necessary. also, the WebFrameClient method shouldn't take a *frame
since it's always 1:1 with a RFImpl. 

[michaelbai, 2014/06/16 21:09:37]

Removed DCHECK().

hmm, as the |frame| parameter, I think it is something new, if I removed the
parameter, it make me difficult to implement related tests in blink as almost
every WebFrameClient method has frame parameter, that's the way to get frame
object in WebFrameTest.

Are you ok to leave the code as it is, and remove all unnecessary frame
parameters as part of refactorying?



On 2014/06/16 20:00:45, jam wrote:

[michaelbai, 2014/06/16 21:09:37]

Done.

[dcheng, 2014/06/16 21:16:54]

That's all legacy code. Since WebFrame has a 1:1 relationship with
WebFrameClient, every WebFrameClient, by definition, should know which WebFrame
it's associated with. That's why the parameter is redundant for new methods.

+  if (frame->parent())

[dcheng, 2014/06/16 17:21:25]

Is there a design doc for this? I take it that we don't want to handle this
except for the top-level frame... IMO that check belongs in Blink where we
process the meta tag.

[michaelbai, 2014/06/16 17:49:23]

Feature description is in the linked bug.

I think we did check whether frame is top-level in this class, you can find
frame->parent() is invoked in many places.



On 2014/06/16 17:21:25, dcheng wrote:

[jam, 2014/06/16 20:00:45]

If blink is checking this (which I agree makes sense to be the place that is
doing so), I don't see why we should check it again here.

[michaelbai, 2014/06/16 21:09:37]

Currently blink isn't checking this, I will move it blink.


On 2014/06/16 20:00:45, jam wrote:

[jam, 2014/06/18 00:36:43]

since per abarth's comments he wants this to be in content, you'll need to bring
back
if (frame_->parent())
  return

[michaelbai, 2014/06/18 03:40:12]

Done.

+    return;
+
+  Send(new FrameHostMsg_DidChangeBrandColor(
+      routing_id_, frame->document().brandColor()));
+}
+
 blink::WebNotificationPresenter* RenderFrameImpl::notificationPresenter() {
   return notification_provider_;
 }
 
 void RenderFrameImpl::didChangeSelection(bool is_empty_selection) {
   if (!GetRenderWidget()->handling_input_event() && !handling_select_range_)
     return;
 
   if (is_empty_selection)
     selection_text_.clear();
@@ skipping 1371 matching lines @@
 
 #if defined(ENABLE_BROWSER_CDMS)
 RendererCdmManager* RenderFrameImpl::GetCdmManager() {
   if (!cdm_manager_)
     cdm_manager_ = new RendererCdmManager(this);
   return cdm_manager_;
 }
 #endif  // defined(ENABLE_BROWSER_CDMS)
 
 }  // namespace content