Notify the change of brand color from renderer to Brower

content/renderer/render_frame_impl.cc

Index: content/renderer/render_frame_impl.cc
diff --git a/content/renderer/render_frame_impl.cc b/content/renderer/render_frame_impl.cc
index 2a8697dc69ab9488f7b946b31dd3a0c26e2be520..651fb49a4560eec2dc337250d8d5eac209844c6d 100644
--- a/content/renderer/render_frame_impl.cc
+++ b/content/renderer/render_frame_impl.cc
@@ -2205,6 +2205,15 @@ void RenderFrameImpl::didUpdateCurrentHistoryItem(blink::WebLocalFrame* frame) {
   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_;
 }