Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2545693002:
Support allowpaymentrequest with out-of-process iframes (Closed)
|
Created:
4 years ago by pals Modified:
4 years ago CC:
chromium-reviews, rouslan+payments_chromium.org, mlamouri+watch-blink_chromium.org, blink-reviews-html_chromium.org, jam, dcheng, blink-reviews-api_chromium.org, haraken, dglazkov+blink, darin-cc_chromium.org, blink-reviews, kinuko+watch, sebsg+paymentswatch_chromium.org, site-isolation-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSupport allowpaymentrequest with out-of-process iframes
BUG=669585
Committed: https://crrev.com/a1f17e817c65d8f3e8ba10beee7b43e625f21c04
Cr-Commit-Position: refs/heads/master@{#438724}
Patch Set 1 #Patch Set 2 : Fixed the error #
Total comments: 1
Patch Set 3 : Added Test #
Total comments: 3
Patch Set 4 : Fix for Test #Patch Set 5 : Try1 #Patch Set 6 : Support allowpaymentrequest with out-of-process iframes #
Total comments: 6
Patch Set 7 : Support allowpaymentrequest with out-of-process iframes #Messages
Total messages: 80 (39 generated)
rouslan@chromium.org changed reviewers: + haraken@chromium.org, rouslan@chromium.org
haraken, is this the correct direction for fixing OOPIF crash? Please point me in the direction of an OOPIF expert that I can consult.
Description was changed from ========== Support allowpaymentrequest with out-of-process iframes BUG=669585 ========== to ========== [DO NOT SUBMIT] Support allowpaymentrequest with out-of-process iframes BUG=669585 ==========
haraken@chromium.org changed reviewers: + dcheng@chromium.org, nasko@chromium.org
+dcheng +nasko
sanjoy.pal@samsung.com changed reviewers: + alexmos@chromium.org
Now, it works fine for OOPIFs also. I'll try add test cases for this change. https://codereview.chromium.org/2545693002/diff/20001/content/common/frame_me... File content/common/frame_messages.h (right): https://codereview.chromium.org/2545693002/diff/20001/content/common/frame_me... content/common/frame_messages.h:177: IPC_STRUCT_TRAITS_MEMBER(allow_payment_request) Missed out this change in the previous patch. Now, it works fine.
Description was changed from ========== [DO NOT SUBMIT] Support allowpaymentrequest with out-of-process iframes BUG=669585 ========== to ========== Support allowpaymentrequest with out-of-process iframes BUG=669585 ==========
Do we need to remove HTMLIFrameElementPayments now? I've confirmed that the bug is fixed in this patch.
This looks good, but is it possible to add a test? Something similar to FrameOwnerPropertiesPropagationCSP or AllowFullscreen in site_per_process_browsertest.cc? I don't know if a document can easily check whether it's allowed to request payments the way this is possible for fullscreen, but we can at least check that we don't crash and that the allow_payment_requests is propagated properly to the FrameReplicationState in the browser process. Also, it's sad that we need to add yet another allow* iframe attribute like this. Presumably there are plans to eventually migrate this to feature policy? (I saw some discussion of this on blink-dev but haven't followed it closely.)
On 2016/12/02 17:40:28, alexmos wrote: > This looks good, but is it possible to add a test? Something similar to > FrameOwnerPropertiesPropagationCSP or AllowFullscreen in > site_per_process_browsertest.cc? I don't know if a document can easily check > whether it's allowed to request payments the way this is possible for > fullscreen, but we can at least check that we don't crash and that the > allow_payment_requests is propagated properly to the FrameReplicationState in > the browser process. pals, I've sketched out a test in http://crrev.com/2546143002, but it's not complete. The browsertest.cc file may need to be moved to chrome/browser/payments/? Please take that and run with it. > Also, it's sad that we need to add yet another allow* iframe attribute like > this. Presumably there are plans to eventually migrate this to feature policy? > (I saw some discussion of this on blink-dev but haven't followed it closely.) Other browsers don't yet have a plan to support FeaturePolicy, so we can't use it just yet. The current plan is: 1) Everyone implement allowpaymentrequest. 2) Chrome ships FeaturePolicy, which has enable="paymentrequest". 3) Chrome's implementation of allowpaymentrequest uses FeaturePolicy code path. 4) Everyone implements FeaturePolicy. 5) allowpaymentrequest is deprecated in favor of FeaturePolicy. We're in step 1 right now. Steps 2-3 are 2017Q1, probably. No word of when Step 4 will take place.
The CQ bit was checked by sanjoy.pal@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
sanjoy.pal@samsung.com changed reviewers: + sky@chromium.org
Can we serve iframe html from different domain for TYPE_HTTPS? https://codereview.chromium.org/2545693002/diff/60001/chrome/browser/payments... File chrome/browser/payments/site_per_process_payments_browsertest.cc (right): https://codereview.chromium.org/2545693002/diff/60001/chrome/browser/payments... chrome/browser/payments/site_per_process_payments_browsertest.cc:57: EXPECT_NE(frame->GetSiteInstance(), tab->GetMainFrame()->GetSiteInstance()); This test is failing because iframe is not creating a new process as iframe is not served from a different domain.
Patchset #3 (id:40001) has been deleted
The CQ bit was checked by sanjoy.pal@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
https://codereview.chromium.org/2545693002/diff/60001/chrome/browser/payments... File chrome/browser/payments/site_per_process_payments_browsertest.cc (right): https://codereview.chromium.org/2545693002/diff/60001/chrome/browser/payments... chrome/browser/payments/site_per_process_payments_browsertest.cc:57: EXPECT_NE(frame->GetSiteInstance(), tab->GetMainFrame()->GetSiteInstance()); On 2016/12/06 09:01:28, pals wrote: > This test is failing because iframe is not creating a new process as iframe is > not served from a different domain. You need to do something like this: // Navigate the iframe cross-site. TestNavigationObserver load_observer(shell()->web_contents()); GURL frame_url = embedded_test_server()->GetURL( "b.com", "/payment_request_iframe.html"); EXPECT_TRUE( ExecuteScript(root->child_at(0)->current_frame_host(), "window.location.href = '" + frame_url.spec() + "';")); load_observer.Wait(); See: https://cs.chromium.org/chromium/src/content/browser/site_per_process_browser...
On 2016/12/06 13:24:47, rouslan wrote: > https://codereview.chromium.org/2545693002/diff/60001/chrome/browser/payments... > File chrome/browser/payments/site_per_process_payments_browsertest.cc (right): > > https://codereview.chromium.org/2545693002/diff/60001/chrome/browser/payments... > chrome/browser/payments/site_per_process_payments_browsertest.cc:57: > EXPECT_NE(frame->GetSiteInstance(), tab->GetMainFrame()->GetSiteInstance()); > On 2016/12/06 09:01:28, pals wrote: > > This test is failing because iframe is not creating a new process as iframe is > > not served from a different domain. > > You need to do something like this: > > // Navigate the iframe cross-site. > TestNavigationObserver load_observer(shell()->web_contents()); > GURL frame_url = embedded_test_server()->GetURL( > "b.com", "/payment_request_iframe.html"); > EXPECT_TRUE( > ExecuteScript(root->child_at(0)->current_frame_host(), > "window.location.href = '" + frame_url.spec() + "';")); > load_observer.Wait(); > > See: > > https://cs.chromium.org/chromium/src/content/browser/site_per_process_browser... I tried that, but its not working for TYPE_HTTPS server.
It's not clear why you added me to this patch. If it's for c/t/BUILD.gn that LGTM. I'll also point out that c/t/BUILD.gn has an OWNERS of *, so in the future don't feel you need to add me just for a BUILD.gn change. If there is another file you need me to review please indicate it. Thanks.
https://codereview.chromium.org/2545693002/diff/60001/chrome/browser/payments... File chrome/browser/payments/site_per_process_payments_browsertest.cc (right): https://codereview.chromium.org/2545693002/diff/60001/chrome/browser/payments... chrome/browser/payments/site_per_process_payments_browsertest.cc:57: EXPECT_NE(frame->GetSiteInstance(), tab->GetMainFrame()->GetSiteInstance()); On 2016/12/06 13:24:47, rouslan wrote: > On 2016/12/06 09:01:28, pals wrote: > > This test is failing because iframe is not creating a new process as iframe is > > not served from a different domain. > > You need to do something like this: > > // Navigate the iframe cross-site. > TestNavigationObserver load_observer(shell()->web_contents()); > GURL frame_url = embedded_test_server()->GetURL( > "b.com", "/payment_request_iframe.html"); > EXPECT_TRUE( > ExecuteScript(root->child_at(0)->current_frame_host(), > "window.location.href = '" + frame_url.spec() + "';")); > load_observer.Wait(); > > See: > > https://cs.chromium.org/chromium/src/content/browser/site_per_process_browser... Why do you need to use the HTTPS server? If it's really needed, I think Rouslan's suggestion should work, or alternatively you could also do NavigateIframeToURL with that URL, or use the redirector in the HTML directly: <iframe src="/cross-site/b.com/foo.html"> (see basic-active-in-iframe.html and the ActiveMixedContentInIframe test for an example). But also, why does it need to be in chrome/? Does the crash not repro from a content test?
https://codereview.chromium.org/2545693002/diff/60001/chrome/browser/payments... File chrome/browser/payments/site_per_process_payments_browsertest.cc (right): https://codereview.chromium.org/2545693002/diff/60001/chrome/browser/payments... chrome/browser/payments/site_per_process_payments_browsertest.cc:57: EXPECT_NE(frame->GetSiteInstance(), tab->GetMainFrame()->GetSiteInstance()); On 2016/12/06 13:24:47, rouslan wrote: > On 2016/12/06 09:01:28, pals wrote: > > This test is failing because iframe is not creating a new process as iframe is > > not served from a different domain. > > You need to do something like this: > > // Navigate the iframe cross-site. > TestNavigationObserver load_observer(shell()->web_contents()); > GURL frame_url = embedded_test_server()->GetURL( > "b.com", "/payment_request_iframe.html"); > EXPECT_TRUE( > ExecuteScript(root->child_at(0)->current_frame_host(), > "window.location.href = '" + frame_url.spec() + "';")); > load_observer.Wait(); > > See: > > https://cs.chromium.org/chromium/src/content/browser/site_per_process_browser... Why do you need to use the HTTPS server? If it's really needed, I think Rouslan's suggestion should work, or alternatively you could also do NavigateIframeToURL with that URL, or use the redirector in the HTML directly: <iframe src="/cross-site/b.com/foo.html"> (see basic-active-in-iframe.html and the ActiveMixedContentInIframe test for an example). But also, why does it need to be in chrome/? Does the crash not repro from a content test?
On 2016/12/06 17:50:40, alexmos wrote: > Why do you need to use the HTTPS server? PaymentRequest is available only in a secure context, .e.g., HTTPS. > But also, why does it need to be in chrome/? The crash reproduces from a content test, but a content test will also print out to console how it's not able to connect to the mojo service. Is that OK?
On 2016/12/06 17:58:21, rouslan wrote: > On 2016/12/06 17:50:40, alexmos wrote: > > Why do you need to use the HTTPS server? > > PaymentRequest is available only in a secure context, .e.g., HTTPS. > > > But also, why does it need to be in chrome/? > > The crash reproduces from a content test, but a content test will also print out > to console how it's not able to connect to the mojo service. Is that OK? Is this for a payments mojo service? If so, I agree it's probably better to have a chrome/ test. Note that we already have chrome_site_per_process_browsertest.cc, so you could just put your SitePerProcessPaymentsBrowserTest there (deriving from ChromeSitePerProcessTest which already has --site-per-process enabled).
On 2016/12/06 18:07:58, alexmos (OOO until 12-8) wrote: > Is this for a payments mojo service? Yep.
I took a stab at fixing the browser test in http://crrev.com/2562713002 , but my file layout seems wrong. I've pinged Alex there to help dissect the issue.
On 2016/12/08 15:17:37, rouslan wrote: > I took a stab at fixing the browser test in http://crrev.com/2562713002 , but my > file layout seems wrong. I've pinged Alex there to help dissect the issue. Thanks, I just replied on that CL with what I think the issue is.
Patchset #4 (id:80001) has been deleted
The CQ bit was checked by sanjoy.pal@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Thank you for the help in fixing the test. I had 2 weeks old code where this test was working after adding command_line->AppendSwitch(switches::kIgnoreCertificateErrors); But, after updating the code base the test is crashing as seen in the trybots. #3 0x000001a5b9e1 payments::PaymentRequestImpl::Cancel() #4 0x00000271f4cd payments::PaymentRequestDialog::Cancel() #5 0x7ff67a3cac84 views::DialogClientView::CanClose() #6 0x7ff67a3c4e1b views::Widget::Close() #7 0x0000024093dd web_modal::WebContentsModalDialogManager::WebContentsDestroyed() #8 0x7ff678d4c163 content::WebContentsImpl::~WebContentsImpl() #9 0x7ff678d4cc09 content::WebContentsImpl::~WebContentsImpl() @rouslan, any recent change might cause this? I'm also debugging. Please let me if you suspect any change.
On 2016/12/09 13:05:20, pals wrote: > Thank you for the help in fixing the test. > I had 2 weeks old code where this test was working after adding > command_line->AppendSwitch(switches::kIgnoreCertificateErrors); > But, after updating the code base the test is crashing as seen in the trybots. > > #3 0x000001a5b9e1 payments::PaymentRequestImpl::Cancel() > #4 0x00000271f4cd payments::PaymentRequestDialog::Cancel() > #5 0x7ff67a3cac84 views::DialogClientView::CanClose() > #6 0x7ff67a3c4e1b views::Widget::Close() > #7 0x0000024093dd > web_modal::WebContentsModalDialogManager::WebContentsDestroyed() > #8 0x7ff678d4c163 content::WebContentsImpl::~WebContentsImpl() > #9 0x7ff678d4cc09 content::WebContentsImpl::~WebContentsImpl() > > > @rouslan, any recent change might cause this? I'm also debugging. Please let me > if you suspect any change. This is the CL https://codereview.chromium.org/2529733002/.
On 2016/12/09 13:31:32, pals wrote: > On 2016/12/09 13:05:20, pals wrote: > > Thank you for the help in fixing the test. > > I had 2 weeks old code where this test was working after adding > > command_line->AppendSwitch(switches::kIgnoreCertificateErrors); > > But, after updating the code base the test is crashing as seen in the trybots. > > > > #3 0x000001a5b9e1 payments::PaymentRequestImpl::Cancel() > > #4 0x00000271f4cd payments::PaymentRequestDialog::Cancel() > > #5 0x7ff67a3cac84 views::DialogClientView::CanClose() > > #6 0x7ff67a3c4e1b views::Widget::Close() > > #7 0x0000024093dd > > web_modal::WebContentsModalDialogManager::WebContentsDestroyed() > > #8 0x7ff678d4c163 content::WebContentsImpl::~WebContentsImpl() > > #9 0x7ff678d4cc09 content::WebContentsImpl::~WebContentsImpl() > > > > > > @rouslan, any recent change might cause this? I'm also debugging. Please let > me > > if you suspect any change. > > This is the CL https://codereview.chromium.org/2529733002/. or, may be this one https://codereview.chromium.org/2463453002
On 2016/12/09 13:41:28, pals wrote: > On 2016/12/09 13:31:32, pals wrote: > > On 2016/12/09 13:05:20, pals wrote: > > > Thank you for the help in fixing the test. > > > I had 2 weeks old code where this test was working after adding > > > command_line->AppendSwitch(switches::kIgnoreCertificateErrors); > > > But, after updating the code base the test is crashing as seen in the > trybots. > > > > > > #3 0x000001a5b9e1 payments::PaymentRequestImpl::Cancel() > > > #4 0x00000271f4cd payments::PaymentRequestDialog::Cancel() > > > #5 0x7ff67a3cac84 views::DialogClientView::CanClose() > > > #6 0x7ff67a3c4e1b views::Widget::Close() > > > #7 0x0000024093dd > > > web_modal::WebContentsModalDialogManager::WebContentsDestroyed() > > > #8 0x7ff678d4c163 content::WebContentsImpl::~WebContentsImpl() > > > #9 0x7ff678d4cc09 content::WebContentsImpl::~WebContentsImpl() > > > > > > > > > @rouslan, any recent change might cause this? I'm also debugging. Please let > > me > > > if you suspect any change. > > > > This is the CL https://codereview.chromium.org/2529733002/. > > or, may be this one https://codereview.chromium.org/2463453002 Add "if (client_)" check in https://cs.chromium.org/chromium/src/chrome/browser/payments/payment_request_... . That's desktop implementation, which is not complete yet. They don't handle tab closing yet.
The CQ bit was checked by sanjoy.pal@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
On 2016/12/09 13:45:39, rouslan wrote: > Add "if (client_)" check in > https://cs.chromium.org/chromium/src/chrome/browser/payments/payment_request_... > . That's desktop implementation, which is not complete yet. They don't handle > tab closing yet. Tried my own advice and realized it's wrong, because "client_" is not a pointer. I'm not sure why it's crashing exactly, but you can fix it by moving the "chrome::ShowPaymentRequestDialog(this)" call from PaymentRequestImpl::Init() to PaymentRequestImpl::Show(), where it should be. I've verified that fixes the browser test. See https://codereview.chromium.org/2568123002.
The CQ bit was checked by sanjoy.pal@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thank you for the help in fixing the issue. PTAL.
LGTM. Great work!
Thanks for adding and fixing the test! LGTM with nits. [also, +site-isolation-reviews@, which I forgot to add earlier] https://codereview.chromium.org/2545693002/diff/140001/chrome/browser/payment... File chrome/browser/payments/site_per_process_payments_browsertest.cc (right): https://codereview.chromium.org/2545693002/diff/140001/chrome/browser/payment... chrome/browser/payments/site_per_process_payments_browsertest.cc:33: command_line->AppendSwitch(switches::kIgnoreCertificateErrors); Perhaps add a quick comment mentioning why this switch is needed? (I.e., that the HTTPS server only serves a valid cert for localhost, so this is needed to load pages from other hosts without an error.) https://codereview.chromium.org/2545693002/diff/140001/chrome/browser/payment... chrome/browser/payments/site_per_process_payments_browsertest.cc:49: https_server.StartAcceptingConnections(); The six lines above might belong better in SetUpOnMainThread() which you can override in SitePerProcessPaymentsBrowserTest. That way you won't have to repeat this if you add more test cases later. https://codereview.chromium.org/2545693002/diff/140001/chrome/test/data/payme... File chrome/test/data/payment_request_main.html (right): https://codereview.chromium.org/2545693002/diff/140001/chrome/test/data/payme... chrome/test/data/payment_request_main.html:2: <iframe src="payment_request_iframe.html" id="test" allowpaymentrequest></iframe> This is fine to keep if you want, but you don't really need the src here since you will end up navigating the iframe from your test, right?
The CQ bit was checked by sanjoy.pal@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Fixed the comments. Please take a look. https://codereview.chromium.org/2545693002/diff/140001/chrome/browser/payment... File chrome/browser/payments/site_per_process_payments_browsertest.cc (right): https://codereview.chromium.org/2545693002/diff/140001/chrome/browser/payment... chrome/browser/payments/site_per_process_payments_browsertest.cc:33: command_line->AppendSwitch(switches::kIgnoreCertificateErrors); On 2016/12/13 17:26:56, alexmos wrote: > Perhaps add a quick comment mentioning why this switch is needed? (I.e., that > the HTTPS server only serves a valid cert for localhost, so this is needed to > load pages from other hosts without an error.) Done. https://codereview.chromium.org/2545693002/diff/140001/chrome/browser/payment... chrome/browser/payments/site_per_process_payments_browsertest.cc:49: https_server.StartAcceptingConnections(); On 2016/12/13 17:26:56, alexmos wrote: > The six lines above might belong better in SetUpOnMainThread() which you can > override in SitePerProcessPaymentsBrowserTest. That way you won't have to > repeat this if you add more test cases later. Done. https://codereview.chromium.org/2545693002/diff/140001/chrome/test/data/payme... File chrome/test/data/payment_request_main.html (right): https://codereview.chromium.org/2545693002/diff/140001/chrome/test/data/payme... chrome/test/data/payment_request_main.html:2: <iframe src="payment_request_iframe.html" id="test" allowpaymentrequest></iframe> On 2016/12/13 17:26:56, alexmos wrote: > This is fine to keep if you want, but you don't really need the src here since > you will end up navigating the iframe from your test, right? Done.
Thanks, LGTM
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by sanjoy.pal@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org, rouslan@chromium.org Link to the patchset: https://codereview.chromium.org/2545693002/#ps160001 (title: "Support allowpaymentrequest with out-of-process iframes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
On 2016/12/14 05:40:38, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) @nasko, PTAL for c/c/frame_messages.h
IPC LGTM
The CQ bit was checked by sanjoy.pal@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 160001, "attempt_start_ts": 1481772839970180,
"parent_rev": "37629c3ed758f6b944b0f0787b3540e81d840bd1", "commit_rev":
"453492f0fb1f259710609e1a44cba6c4244b30ef"}
Message was sent while issue was closed.
Description was changed from ========== Support allowpaymentrequest with out-of-process iframes BUG=669585 ========== to ========== Support allowpaymentrequest with out-of-process iframes BUG=669585 Review-Url: https://codereview.chromium.org/2545693002 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Support allowpaymentrequest with out-of-process iframes BUG=669585 Review-Url: https://codereview.chromium.org/2545693002 ========== to ========== Support allowpaymentrequest with out-of-process iframes BUG=669585 Committed: https://crrev.com/a1f17e817c65d8f3e8ba10beee7b43e625f21c04 Cr-Commit-Position: refs/heads/master@{#438724} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/a1f17e817c65d8f3e8ba10beee7b43e625f21c04 Cr-Commit-Position: refs/heads/master@{#438724}
Message was sent while issue was closed.
I noticed this CL didn't get a L-G-T-M from a blink OWNER. I think this is because sky is in //third_party/OWNERS. While the CL does LGTM, let's be careful in the future (sky only approved the //content changes)
Message was sent while issue was closed.
This is a crash fix for OOPIF. IFrame support is a status=experimental runtime-enabled feature. Did this patch accidentally enable it?
Message was sent while issue was closed.
Disabling iframe support again in http://crrev.com/2580893002
Message was sent while issue was closed.
Thanks for catching this dcheng. Indeed, I'm not an owner of WebKit. I'm going to see about adding "set noparent" to WebKit/OWNERS On Wed, Dec 14, 2016 at 10:12 PM, <dcheng@chromium.org> wrote: > I noticed this CL didn't get a L-G-T-M from a blink OWNER. I think this is > because sky is in //third_party/OWNERS. > > While the CL does LGTM, let's be careful in the future (sky only approved > the > //content changes) > > https://codereview.chromium.org/2545693002/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
Thanks for catching this dcheng. Indeed, I'm not an owner of WebKit. I'm going to see about adding "set noparent" to WebKit/OWNERS On Wed, Dec 14, 2016 at 10:12 PM, <dcheng@chromium.org> wrote: > I noticed this CL didn't get a L-G-T-M from a blink OWNER. I think this is > because sky is in //third_party/OWNERS. > > While the CL does LGTM, let's be careful in the future (sky only approved > the > //content changes) > > https://codereview.chromium.org/2545693002/ -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
https://codereview.chromium.org/2582593002/ On Thu, Dec 15, 2016 at 9:00 AM, Scott Violet <sky@chromium.org> wrote: > Thanks for catching this dcheng. Indeed, I'm not an owner of WebKit. > I'm going to see about adding "set noparent" to WebKit/OWNERS > > On Wed, Dec 14, 2016 at 10:12 PM, <dcheng@chromium.org> wrote: >> I noticed this CL didn't get a L-G-T-M from a blink OWNER. I think this is >> because sky is in //third_party/OWNERS. >> >> While the CL does LGTM, let's be careful in the future (sky only approved >> the >> //content changes) >> >> https://codereview.chromium.org/2545693002/ -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
https://codereview.chromium.org/2582593002/ On Thu, Dec 15, 2016 at 9:00 AM, Scott Violet <sky@chromium.org> wrote: > Thanks for catching this dcheng. Indeed, I'm not an owner of WebKit. > I'm going to see about adding "set noparent" to WebKit/OWNERS > > On Wed, Dec 14, 2016 at 10:12 PM, <dcheng@chromium.org> wrote: >> I noticed this CL didn't get a L-G-T-M from a blink OWNER. I think this is >> because sky is in //third_party/OWNERS. >> >> While the CL does LGTM, let's be careful in the future (sky only approved >> the >> //content changes) >> >> https://codereview.chromium.org/2545693002/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org. |
