Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
339133004:
Removed the frame parameter from didChangeBrandColor (Closed)
|
Created:
6 years, 6 months ago by michaelbai Modified:
6 years, 6 months ago CC:
blink-reviews, dglazkov+blink, jam, jamesr Base URL:
svn://svn.chromium.org/blink/trunk Project:
blink Visibility:
Public. |
DescriptionRemoved the frame parameter from didChangeBrandColor because
the RenderFrame and WebFrame has 1:1 mapping now
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=176459
Patch Set 1 #
Total comments: 2
Patch Set 2 : address comment #
Total comments: 3
Patch Set 3 : Send notification unconditionally #
Messages
Total messages: 16 (0 generated)
https://codereview.chromium.org/339133004/diff/1/Source/web/tests/WebFrameTes... File Source/web/tests/WebFrameTest.cpp (right): https://codereview.chromium.org/339133004/diff/1/Source/web/tests/WebFrameTes... Source/web/tests/WebFrameTest.cpp:5575: EXPECT_EQ(0xff0000ff, webViewHelper.webView()->mainFrame()->document().brandColor()); The site isolation team hasn't had enough cycles to start moving legacy methods from WebFrame to WebLocalFrame, but I would recommend going through mainFrameImpl() for now here--WebDocument isn't going to be available from WebFrame in the long term, and it saves us some refactoring later.
PTAL https://codereview.chromium.org/339133004/diff/1/Source/web/tests/WebFrameTes... File Source/web/tests/WebFrameTest.cpp (right): https://codereview.chromium.org/339133004/diff/1/Source/web/tests/WebFrameTes... Source/web/tests/WebFrameTest.cpp:5575: EXPECT_EQ(0xff0000ff, webViewHelper.webView()->mainFrame()->document().brandColor()); On 2014/06/17 16:51:55, dcheng wrote: > The site isolation team hasn't had enough cycles to start moving legacy methods > from WebFrame to WebLocalFrame, but I would recommend going through > mainFrameImpl() for now here--WebDocument isn't going to be available from > WebFrame in the long term, and it saves us some refactoring later. Done. Is there any document about how to add web API in blink from site isolation team?
On 2014/06/17 at 17:06:09, michaelbai wrote: > PTAL > > https://codereview.chromium.org/339133004/diff/1/Source/web/tests/WebFrameTes... > File Source/web/tests/WebFrameTest.cpp (right): > > https://codereview.chromium.org/339133004/diff/1/Source/web/tests/WebFrameTes... > Source/web/tests/WebFrameTest.cpp:5575: EXPECT_EQ(0xff0000ff, webViewHelper.webView()->mainFrame()->document().brandColor()); > On 2014/06/17 16:51:55, dcheng wrote: > > The site isolation team hasn't had enough cycles to start moving legacy methods > > from WebFrame to WebLocalFrame, but I would recommend going through > > mainFrameImpl() for now here--WebDocument isn't going to be available from > > WebFrame in the long term, and it saves us some refactoring later. > > Done. > > Is there any document about how to add web API in blink from site isolation team? Unfortunately, not yet. We're hoping to land the basic infrastructure so other teams can test their feature out with out-of-process iframes by the end of quarter. We're also still exploring the best patterns/practices, and we're getting some firsthand experience about this ourselves. If you're curious about an overview of how OOPI works, there is the BlinkOn 2 presentation: https://docs.google.com/presentation/d/1DIMK7eTZSVM6B7B48OHtckiAqT__Ua2efzkC7... (The change itself LGTM but I am not an owner in any of the directories so you'll need abarth)
On 2014/06/17 18:19:52, dcheng wrote: > On 2014/06/17 at 17:06:09, michaelbai wrote: > > PTAL > > > > > https://codereview.chromium.org/339133004/diff/1/Source/web/tests/WebFrameTes... > > File Source/web/tests/WebFrameTest.cpp (right): > > > > > https://codereview.chromium.org/339133004/diff/1/Source/web/tests/WebFrameTes... > > Source/web/tests/WebFrameTest.cpp:5575: EXPECT_EQ(0xff0000ff, > webViewHelper.webView()->mainFrame()->document().brandColor()); > > On 2014/06/17 16:51:55, dcheng wrote: > > > The site isolation team hasn't had enough cycles to start moving legacy > methods > > > from WebFrame to WebLocalFrame, but I would recommend going through > > > mainFrameImpl() for now here--WebDocument isn't going to be available from > > > WebFrame in the long term, and it saves us some refactoring later. > > > > Done. > > > > Is there any document about how to add web API in blink from site isolation > team? > > Unfortunately, not yet. We're hoping to land the basic infrastructure so other > teams can test their feature out with out-of-process iframes by the end of > quarter. We're also still exploring the best patterns/practices, and we're > getting some firsthand experience about this ourselves. If you're curious about > an overview of how OOPI works, there is the BlinkOn 2 presentation: > https://docs.google.com/presentation/d/1DIMK7eTZSVM6B7B48OHtckiAqT__Ua2efzkC7... > > (The change itself LGTM but I am not an owner in any of the directories so > you'll need abarth) It seemed that you already has some patterns, like the changes in this patch, - WebFrameClient's method shouldn't take the frame parameter. - Using mainFrameImpl instead of mainFrame etc. At least, you should let the blink reviewers know about these, so somebody else doesn't have to go back and forth, like I did here.
https://codereview.chromium.org/339133004/diff/20001/Source/web/FrameLoaderCl... File Source/web/FrameLoaderClientImpl.cpp (right): https://codereview.chromium.org/339133004/diff/20001/Source/web/FrameLoaderCl... Source/web/FrameLoaderClientImpl.cpp:456: m_webFrame->client()->didChangeBrandColor(); This change isn't correct. You're welcome to ignore the notification on the Chromium side for non-top-level frames if you don't care about it. https://codereview.chromium.org/339133004/diff/20001/public/web/WebFrameClient.h File public/web/WebFrameClient.h (right): https://codereview.chromium.org/339133004/diff/20001/public/web/WebFrameClien... public/web/WebFrameClient.h:270: virtual void didChangeBrandColor() { } This change isn't correct. We always pass WebLocalFrame* to WebFrameClient functions. You're welcome to ignore the argument on the Chromium side.
I don't feel strongly about which side of the API boundary we check that it's a top-level frame, but why pass Frame* to a WebFrameClient method?
On 2014/06/17 at 21:55:25, dcheng wrote: > I don't feel strongly about which side of the API boundary we check that it's a top-level frame, but why pass Frame* to a WebFrameClient method? (I mean a WebLocalFrame*)
+jam, who also suggested these changes.
https://codereview.chromium.org/339133004/diff/20001/public/web/WebFrameClient.h File public/web/WebFrameClient.h (right): https://codereview.chromium.org/339133004/diff/20001/public/web/WebFrameClien... public/web/WebFrameClient.h:270: virtual void didChangeBrandColor() { } On 2014/06/17 21:52:48, abarth wrote: > This change isn't correct. We always pass WebLocalFrame* to WebFrameClient > functions. You're welcome to ignore the argument on the Chromium side. Adam: this was originally done since the WebFrameClient implementation on the content side (RenderViewImpl) responded to many WebFrames. Now, there is one RenderFrameImpl per WebFrameClient, so passing a pointer to the frame is redundant. We are planning on removing all these, but were going to wait for the repo merge since it'll be trivial to do it then as compared to now. but for new APIs, I don't see much reason. you can see that for many of the methods that we've moved here from WebViewClient, we have avoided that extra parameter.
On 2014/06/17 at 22:45:05, jam wrote: > Adam: this was originally done since the WebFrameClient implementation on the content side (RenderViewImpl) responded to many WebFrames. Now, there is one RenderFrameImpl per WebFrameClient, so passing a pointer to the frame is redundant. We are planning on removing all these, but were going to wait for the repo merge since it'll be trivial to do it then as compared to now. but for new APIs, I don't see much reason. you can see that for many of the methods that we've moved here from WebViewClient, we have avoided that extra parameter. Ok, we can remove the parameter. However, we should still send the notification unconditionally.
On 2014/06/17 23:54:44, abarth wrote: > On 2014/06/17 at 22:45:05, jam wrote: > > Adam: this was originally done since the WebFrameClient implementation on the > content side (RenderViewImpl) responded to many WebFrames. Now, there is one > RenderFrameImpl per WebFrameClient, so passing a pointer to the frame is > redundant. We are planning on removing all these, but were going to wait for the > repo merge since it'll be trivial to do it then as compared to now. but for new > APIs, I don't see much reason. you can see that for many of the methods that > we've moved here from WebViewClient, we have avoided that extra parameter. > > Ok, we can remove the parameter. However, we should still send the notification > unconditionally. sgtm, thanks
Send notification unconditionally, PTAL
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/michaelbai@chromium.org/339133004/40001
Message was sent while issue was closed.
Change committed as 176459 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
