Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
331793005:
Notify the change of brand color from renderer to Brower (Closed)
|
Created:
6 years, 6 months ago by michaelbai Modified:
6 years, 6 months ago CC:
chromium-reviews, creis+watch_chromium.org, darin-cc_chromium.org, jam, miu+watch_chromium.org, nasko+codewatch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionThis patch makes the brand color change message available for WebContents Observer.
BUG=381447
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=278792
Patch Set 1 #
Total comments: 15
Patch Set 2 : address comments #Patch Set 3 : remove the frame parameter #Patch Set 4 : check main frame #Patch Set 5 : sync #
Messages
Total messages: 24 (0 generated)
dcheng@ for frame_message.h change
https://codereview.chromium.org/331793005/diff/1/content/renderer/render_fram... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/331793005/diff/1/content/renderer/render_fram... content/renderer/render_frame_impl.cc:2209: DCHECK(!frame_ || frame_ == frame); Can frame_ ever be NULL here? https://codereview.chromium.org/331793005/diff/1/content/renderer/render_fram... content/renderer/render_frame_impl.cc:2210: if (frame->parent()) 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.
PTAL https://codereview.chromium.org/331793005/diff/1/content/renderer/render_fram... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/331793005/diff/1/content/renderer/render_fram... content/renderer/render_frame_impl.cc:2209: DCHECK(!frame_ || frame_ == frame); I don't know, but there is same code in the class, like RenderFrameImpl::didChangeName. On 2014/06/16 17:21:25, dcheng wrote: > Can frame_ ever be NULL here? https://codereview.chromium.org/331793005/diff/1/content/renderer/render_fram... content/renderer/render_frame_impl.cc:2210: if (frame->parent()) 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: > 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.
https://codereview.chromium.org/331793005/diff/1/content/common/frame_messages.h File content/common/frame_messages.h (right): https://codereview.chromium.org/331793005/diff/1/content/common/frame_message... content/common/frame_messages.h:613: uint32 /* brand_color */) SkColor instead of uint32. Ditto for other locations throughout this patch that reference uint32. https://codereview.chromium.org/331793005/diff/1/content/renderer/render_fram... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/331793005/diff/1/content/renderer/render_fram... content/renderer/render_frame_impl.cc:2209: DCHECK(!frame_ || frame_ == frame); On 2014/06/16 17:49:22, michaelbai wrote: > I don't know, but there is same code in the class, like > RenderFrameImpl::didChangeName. > > On 2014/06/16 17:21:25, dcheng wrote: > > Can frame_ ever be NULL here? > 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.
https://codereview.chromium.org/331793005/diff/1/content/renderer/render_fram... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/331793005/diff/1/content/renderer/render_fram... content/renderer/render_frame_impl.cc:2209: DCHECK(!frame_ || frame_ == frame); this isn't necessary. also, the WebFrameClient method shouldn't take a *frame since it's always 1:1 with a RFImpl. https://codereview.chromium.org/331793005/diff/1/content/renderer/render_fram... content/renderer/render_frame_impl.cc:2210: if (frame->parent()) 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.
PTAL https://codereview.chromium.org/331793005/diff/1/content/common/frame_messages.h File content/common/frame_messages.h (right): https://codereview.chromium.org/331793005/diff/1/content/common/frame_message... content/common/frame_messages.h:613: uint32 /* brand_color */) On 2014/06/16 18:52:19, dcheng wrote: > SkColor instead of uint32. Ditto for other locations throughout this patch that > reference uint32. Done. https://codereview.chromium.org/331793005/diff/1/content/renderer/render_fram... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/331793005/diff/1/content/renderer/render_fram... content/renderer/render_frame_impl.cc:2209: DCHECK(!frame_ || frame_ == frame); On 2014/06/16 18:52:19, dcheng wrote: > On 2014/06/16 17:49:22, michaelbai wrote: > > I don't know, but there is same code in the class, like > > RenderFrameImpl::didChangeName. > > > > On 2014/06/16 17:21:25, dcheng wrote: > > > Can frame_ ever be NULL here? > > > > 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. Done. https://codereview.chromium.org/331793005/diff/1/content/renderer/render_fram... content/renderer/render_frame_impl.cc:2209: DCHECK(!frame_ || frame_ == frame); 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: > this isn't necessary. also, the WebFrameClient method shouldn't take a *frame > since it's always 1:1 with a RFImpl. https://codereview.chromium.org/331793005/diff/1/content/renderer/render_fram... content/renderer/render_frame_impl.cc:2210: if (frame->parent()) Currently blink isn't checking this, I will move it blink. On 2014/06/16 20:00:45, jam wrote: > 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.
https://codereview.chromium.org/331793005/diff/1/content/renderer/render_fram... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/331793005/diff/1/content/renderer/render_fram... content/renderer/render_frame_impl.cc:2209: DCHECK(!frame_ || frame_ == frame); On 2014/06/16 21:09:37, michaelbai wrote: > 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: > > this isn't necessary. also, the WebFrameClient method shouldn't take a *frame > > since it's always 1:1 with a RFImpl. > 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.
On 2014/06/16 21:16:54, dcheng wrote: > https://codereview.chromium.org/331793005/diff/1/content/renderer/render_fram... > File content/renderer/render_frame_impl.cc (right): > > https://codereview.chromium.org/331793005/diff/1/content/renderer/render_fram... > content/renderer/render_frame_impl.cc:2209: DCHECK(!frame_ || frame_ == frame); > On 2014/06/16 21:09:37, michaelbai wrote: > > 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: > > > this isn't necessary. also, the WebFrameClient method shouldn't take a > *frame > > > since it's always 1:1 with a RFImpl. > > > > 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. I understand and totally agree that the parameter is redundant, but to have WebFrameClient to know WebFrame is not simple change, and it should be covered by another CL.
On 2014/06/16 at 21:53:03, michaelbai wrote: > On 2014/06/16 21:16:54, dcheng wrote: > > https://codereview.chromium.org/331793005/diff/1/content/renderer/render_fram... > > File content/renderer/render_frame_impl.cc (right): > > > > https://codereview.chromium.org/331793005/diff/1/content/renderer/render_fram... > > content/renderer/render_frame_impl.cc:2209: DCHECK(!frame_ || frame_ == frame); > > On 2014/06/16 21:09:37, michaelbai wrote: > > > 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: > > > > this isn't necessary. also, the WebFrameClient method shouldn't take a > > *frame > > > > since it's always 1:1 with a RFImpl. > > > > > > > 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. > > I understand and totally agree that the parameter is redundant, but to have WebFrameClient to know WebFrame is not simple change, and it should be covered by another CL. I don't follow... can you link to a CL/patch that has the problematic tests?
On 2014/06/16 21:55:36, dcheng wrote: > On 2014/06/16 at 21:53:03, michaelbai wrote: > > On 2014/06/16 21:16:54, dcheng wrote: > > > > https://codereview.chromium.org/331793005/diff/1/content/renderer/render_fram... > > > File content/renderer/render_frame_impl.cc (right): > > > > > > > https://codereview.chromium.org/331793005/diff/1/content/renderer/render_fram... > > > content/renderer/render_frame_impl.cc:2209: DCHECK(!frame_ || frame_ == > frame); > > > On 2014/06/16 21:09:37, michaelbai wrote: > > > > 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: > > > > > this isn't necessary. also, the WebFrameClient method shouldn't take a > > > *frame > > > > > since it's always 1:1 with a RFImpl. > > > > > > > > > > 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. > > > > I understand and totally agree that the parameter is redundant, but to have > WebFrameClient to know WebFrame is not simple change, and it should be covered > by another CL. > > I don't follow... can you link to a CL/patch that has the problematic tests? https://codereview.chromium.org/329943004/
On 2014/06/16 at 21:57:35, michaelbai wrote: > On 2014/06/16 21:55:36, dcheng wrote: > > On 2014/06/16 at 21:53:03, michaelbai wrote: > > > On 2014/06/16 21:16:54, dcheng wrote: > > > > > > https://codereview.chromium.org/331793005/diff/1/content/renderer/render_fram... > > > > File content/renderer/render_frame_impl.cc (right): > > > > > > > > > > https://codereview.chromium.org/331793005/diff/1/content/renderer/render_fram... > > > > content/renderer/render_frame_impl.cc:2209: DCHECK(!frame_ || frame_ == > > frame); > > > > On 2014/06/16 21:09:37, michaelbai wrote: > > > > > 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: > > > > > > this isn't necessary. also, the WebFrameClient method shouldn't take a > > > > *frame > > > > > > since it's always 1:1 with a RFImpl. > > > > > > > > > > > > > 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. > > > > > > I understand and totally agree that the parameter is redundant, but to have > > WebFrameClient to know WebFrame is not simple change, and it should be covered > > by another CL. > > > > I don't follow... can you link to a CL/patch that has the problematic tests? > > > https://codereview.chromium.org/329943004/ You don't need WebLocalFrame at all in that test. Simply get rid of the call to get the brand color in the WebFrameClient callback, and verify it in the test itself: EXPECT_EQ(0xff0000ff, webViewHelper.webViewImpl()->mainFrameImpl()->document().brandColor());
On 2014/06/17 16:39:24, dcheng wrote: > On 2014/06/16 at 21:57:35, michaelbai wrote: > > On 2014/06/16 21:55:36, dcheng wrote: > > > On 2014/06/16 at 21:53:03, michaelbai wrote: > > > > On 2014/06/16 21:16:54, dcheng wrote: > > > > > > > > > https://codereview.chromium.org/331793005/diff/1/content/renderer/render_fram... > > > > > File content/renderer/render_frame_impl.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/331793005/diff/1/content/renderer/render_fram... > > > > > content/renderer/render_frame_impl.cc:2209: DCHECK(!frame_ || frame_ == > > > frame); > > > > > On 2014/06/16 21:09:37, michaelbai wrote: > > > > > > 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: > > > > > > > this isn't necessary. also, the WebFrameClient method shouldn't take > a > > > > > *frame > > > > > > > since it's always 1:1 with a RFImpl. > > > > > > > > > > > > > > > > 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. > > > > > > > > I understand and totally agree that the parameter is redundant, but to > have > > > WebFrameClient to know WebFrame is not simple change, and it should be > covered > > > by another CL. > > > > > > I don't follow... can you link to a CL/patch that has the problematic tests? > > > > > > https://codereview.chromium.org/329943004/ > > You don't need WebLocalFrame at all in that test. Simply get rid of the call to > get the brand color in the WebFrameClient callback, and verify it in the test > itself: > EXPECT_EQ(0xff0000ff, > webViewHelper.webViewImpl()->mainFrameImpl()->document().brandColor()); Thanks, this is what I did in https://codereview.chromium.org/339133004/, which is not a perfect fix.
Removed frame parameter, PTAL
lgtm https://codereview.chromium.org/331793005/diff/1/content/renderer/render_fram... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/331793005/diff/1/content/renderer/render_fram... content/renderer/render_frame_impl.cc:2210: if (frame->parent()) On 2014/06/16 21:09:37, michaelbai wrote: > Currently blink isn't checking this, I will move it blink. > > > On 2014/06/16 20:00:45, jam wrote: > > 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. > since per abarth's comments he wants this to be in content, you'll need to bring back if (frame_->parent()) return
dcheng@ still need your approval for frame_message.h https://codereview.chromium.org/331793005/diff/1/content/renderer/render_fram... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/331793005/diff/1/content/renderer/render_fram... content/renderer/render_frame_impl.cc:2210: if (frame->parent()) On 2014/06/18 00:36:43, jam wrote: > On 2014/06/16 21:09:37, michaelbai wrote: > > Currently blink isn't checking this, I will move it blink. > > > > > > On 2014/06/16 20:00:45, jam wrote: > > > 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. > > > > since per abarth's comments he wants this to be in content, you'll need to bring > back > if (frame_->parent()) > return Done.
content/common/frame_messages.h lgtm
The CQ bit was checked by michaelbai@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/michaelbai@chromium.org/331793005/70001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
The CQ bit was checked by michaelbai@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/michaelbai@chromium.org/331793005/70001
Message was sent while issue was closed.
Change committed as 278792 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
