Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1707233003:
Expose mojo bindings to subframes in layout tests. (Closed)
|
Created:
4 years, 10 months ago by Sam McNally Modified:
4 years, 9 months ago CC:
Aaron Boodman, abarth-chromium, ben+mojo_chromium.org, blink-reviews, chrome-apps-syd-reviews_chromium.org, chromium-reviews, creis+watch_chromium.org, darin (slow to review), darin-cc_chromium.org, jam, mkwst+moarreviews-renderer_chromium.org, mlamouri+watch-content_chromium.org, nasko+codewatch_chromium.org, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org, jochen (gone - plz use gerrit) Base URL:
https://chromium.googlesource.com/chromium/src.git@layout-test-mojom Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionExpose mojo bindings to subframes in layout tests.
Committed: https://crrev.com/e2f999841cbfba72585eef56746ab019c0db3b54
Cr-Commit-Position: refs/heads/master@{#381097}
Patch Set 1 : #
Total comments: 2
Patch Set 2 : #
Total comments: 5
Patch Set 3 : #Patch Set 4 : #
Messages
Total messages: 39 (9 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Description was changed from ========== Make mojo bindings accessible from subframes in layout tests. ========== to ========== Expose mojo bindings to subframes in layout tests. ==========
sammc@chromium.org changed reviewers: + creis@chromium.org, rockot@chromium.org
test lgtm https://codereview.chromium.org/1707233003/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/harness-tests/mojo-helpers.html (right): https://codereview.chromium.org/1707233003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/harness-tests/mojo-helpers.html:89: nit: -vspace
https://codereview.chromium.org/1707233003/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/harness-tests/mojo-helpers.html (right): https://codereview.chromium.org/1707233003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/harness-tests/mojo-helpers.html:89: On 2016/02/19 00:23:45, Ken Rockot wrote: > nit: -vspace Done.
I'm happy with adding these bindings to all frames (not just the main frame) for layout tests, but there seem to be some issues with how this is done. I'm wondering if we can make this more straightforward by moving it out of RenderView and doing it for all RenderFrames uniformly. https://codereview.chromium.org/1707233003/diff/60001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1707233003/diff/60001/content/renderer/render... content/renderer/render_frame_impl.cc:1115: } else if (RenderProcess::current()->GetEnabledBindings() & This doesn't look like it does the right thing. We'll call EnableMojoBindings(true) for the main frame if there aren't WEB_UI bindings and there are MOJO bindings. That itself has subtle behavior because it might not take effect if we already called EnableMojoBindings(false) in RenderViewImpl. We should be able to write this more cleanly. https://codereview.chromium.org/1707233003/diff/60001/content/renderer/render... content/renderer/render_frame_impl.cc:1117: EnableMojoBindings(true /* for_layout_tests */); I don't understand how to infer that it's for layout tests based on the absence of WEB_UI bindings. If another use of MOJO bindings were added (other than WebUI and layout tests), wouldn't all the call sites of this become wrong?
esprehn@chromium.org changed reviewers: + esprehn@chromium.org
All this mojo code being added out in content for bindings seems mislayered. There should be a single idl file inside blink that exposes the mojo guts, and then we should just include the generated .js files. There's way too much of blink calling out into the embedder and asking it to inject lots of script into the page. Gin is slow, and this code often doesn't realize what's allowed inside which callbacks. All of the mojo bindings will need to move into blink. :)
On Fri, Feb 19, 2016 at 10:23 PM, <esprehn@chromium.org> wrote: > All this mojo code being added out in content for bindings seems mislayered. > There should be a single idl file inside blink that exposes the mojo guts, and > then we should just include the generated .js files. > > That sounds reasonable to me. Injecting stuff from content was a quick way to get LayoutTest support prototyped while Blink integration details were still being sorted out, because the Web USB folks wanted to start using it ASAP. We still need a story for loading generated JS bindings in tests though and it's not clear to me what that would look like without some kind of support from content. > > There's way too much of blink calling out into the embedder and asking it to > inject lots of script into the page. Gin is slow, and this code often doesn't > realize what's allowed inside which callbacks. > > All of the mojo bindings will need to move into blink. :) > https://codereview.chromium.org/1707233003/ > > -- 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.
On Fri, Feb 19, 2016 at 10:23 PM, <esprehn@chromium.org> wrote: > All this mojo code being added out in content for bindings seems mislayered. > There should be a single idl file inside blink that exposes the mojo guts, and > then we should just include the generated .js files. > > That sounds reasonable to me. Injecting stuff from content was a quick way to get LayoutTest support prototyped while Blink integration details were still being sorted out, because the Web USB folks wanted to start using it ASAP. We still need a story for loading generated JS bindings in tests though and it's not clear to me what that would look like without some kind of support from content. > > There's way too much of blink calling out into the embedder and asking it to > inject lots of script into the page. Gin is slow, and this code often doesn't > realize what's allowed inside which callbacks. > > All of the mojo bindings will need to move into blink. :) > https://codereview.chromium.org/1707233003/ > > -- 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.
On Feb 19, 2016 10:37 PM, "Ken Rockot" <rockot@chromium.org> wrote: > > On Fri, Feb 19, 2016 at 10:23 PM, <esprehn@chromium.org> wrote: >> >> All this mojo code being added out in content for bindings seems mislayered. >> There should be a single idl file inside blink that exposes the mojo guts, and >> then we should just include the generated .js files. > > That sounds reasonable to me. Injecting stuff from content was a quick way to get LayoutTest support prototyped while Blink integration details were still being sorted out, because the Web USB folks wanted to start using it ASAP. > > We still need a story for loading generated JS bindings in tests though and it's not clear to me what that would look like without some kind of support from content. > >> >> There's way too much of blink calling out into the embedder and asking it to >> inject lots of script into the page. Gin is slow, and this code often doesn't >> realize what's allowed inside which callbacks. >> >> All of the mojo bindings will need to move into blink. :) This might be a bit overzealous. Maybe we need a separate support mechanism for mojo bindings in blink, fine, but it is not reasonable to assume that a JS mojo consumer will always need or want a DOM/blink dependency. >> >> https://codereview.chromium.org/1707233003/ > > -- 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.
On Feb 19, 2016 10:37 PM, "Ken Rockot" <rockot@chromium.org> wrote: > > On Fri, Feb 19, 2016 at 10:23 PM, <esprehn@chromium.org> wrote: >> >> All this mojo code being added out in content for bindings seems mislayered. >> There should be a single idl file inside blink that exposes the mojo guts, and >> then we should just include the generated .js files. > > That sounds reasonable to me. Injecting stuff from content was a quick way to get LayoutTest support prototyped while Blink integration details were still being sorted out, because the Web USB folks wanted to start using it ASAP. > > We still need a story for loading generated JS bindings in tests though and it's not clear to me what that would look like without some kind of support from content. > >> >> There's way too much of blink calling out into the embedder and asking it to >> inject lots of script into the page. Gin is slow, and this code often doesn't >> realize what's allowed inside which callbacks. >> >> All of the mojo bindings will need to move into blink. :) This might be a bit overzealous. Maybe we need a separate support mechanism for mojo bindings in blink, fine, but it is not reasonable to assume that a JS mojo consumer will always need or want a DOM/blink dependency. >> >> https://codereview.chromium.org/1707233003/ > > -- 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.
https://codereview.chromium.org/1707233003/diff/60001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1707233003/diff/60001/content/renderer/render... content/renderer/render_frame_impl.cc:1115: } else if (RenderProcess::current()->GetEnabledBindings() & On 2016/02/19 06:26:34, Charlie Reis (slow til Mar 7) wrote: > This doesn't look like it does the right thing. > > We'll call EnableMojoBindings(true) for the main frame if there aren't WEB_UI > bindings and there are MOJO bindings. That itself has subtle behavior because > it might not take effect if we already called EnableMojoBindings(false) in > RenderViewImpl. > > We should be able to write this more cleanly. I don't think that's possible; RenderViewImpl will only call EnableMojoBindings(false) if BINDINGS_POLICY_WEB_UI is enabled. It does feel brittle spread across RenderViewImpl and RenderFrameImpl though. https://codereview.chromium.org/1707233003/diff/60001/content/renderer/render... content/renderer/render_frame_impl.cc:1117: EnableMojoBindings(true /* for_layout_tests */); On 2016/02/19 06:26:34, Charlie Reis (slow til Mar 7) wrote: > I don't understand how to infer that it's for layout tests based on the absence > of WEB_UI bindings. If another use of MOJO bindings were added (other than > WebUI and layout tests), wouldn't all the call sites of this become wrong? The way JS mojo bindings work in content is a bit strange: BINDINGS_POLICY_WEB_UI enables mojo for WebUI; BINDINGS_POLICY_MOJO enables mojo for layout tests. Enabling both at the same time won't work, but nothing tries to do that. If another use of mojo is added, or if the combination of BINDINGS_POLICY_WEB_UI and BINDINGS_POLICY_MOJO starts being used, then this will be wrong, but the same would be true of the similar code in RenderViewImpl::OnAllowBindings(). These two mojo uses each require a different strategy to load generated JS mojo bindings (and extensions is doing its own thing for loading too), so adding a new use is fairly non-trivial.
https://codereview.chromium.org/1707233003/diff/60001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1707233003/diff/60001/content/renderer/render... content/renderer/render_frame_impl.cc:1117: EnableMojoBindings(true /* for_layout_tests */); There should be no process that contains v8 mojo bindings and doesn't include blink. Blink provides idle callback handling, memory pressure, fast JS bindings, and more. Using v8 without blink is not good, and the existing Gin bindings really need to get migrated. This callback out into the embedder that web UI is using is a hack, and should be removed. Blink should have the low level mojo functions Send(), etc. in actual blink idl files. Web UI should just include generated JS files using http requests. This EnableMojoBindings thing needs to go away.
On Wed, Feb 24, 2016 at 12:44 PM, <esprehn@chromium.org> wrote: > > > https://codereview.chromium.org/1707233003/diff/60001/content/renderer/render... > File content/renderer/render_frame_impl.cc (right): > > > https://codereview.chromium.org/1707233003/diff/60001/content/renderer/render... > content/renderer/render_frame_impl.cc:1117: EnableMojoBindings(true /* > for_layout_tests */); > There should be no process that contains v8 mojo bindings and doesn't > include blink. Blink provides idle callback handling, memory pressure, > fast JS bindings, and more. Using v8 without blink is not good, and the > existing Gin bindings really need to get migrated. > > This callback out into the embedder that web UI is using is a hack, and > should be removed. Blink should have the low level mojo functions > Send(), etc. in actual blink idl files. Web UI should just include > generated JS files using http requests. This EnableMojoBindings thing > needs to go away. > It's great if we get benefits from using v8+Blink, and it's fine if Blink IDL + impl is is the only way in which Mojo + JS is ever exposed within Chrome, content, whatever. No problems with that. Getting rid of the EnableMojoBindings hack would be great. I do want to make sure it's clear though, we are not in agreement that a v8 dependency must imply a Blink dependency, and I'm not interested in changing Mojo such that it is ever strictly necessary to depend on Blink in order to use some JS bindings. > https://codereview.chromium.org/1707233003/ > -- 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.
On Wed, Feb 24, 2016 at 12:44 PM, <esprehn@chromium.org> wrote: > > > https://codereview.chromium.org/1707233003/diff/60001/content/renderer/render... > File content/renderer/render_frame_impl.cc (right): > > > https://codereview.chromium.org/1707233003/diff/60001/content/renderer/render... > content/renderer/render_frame_impl.cc:1117: EnableMojoBindings(true /* > for_layout_tests */); > There should be no process that contains v8 mojo bindings and doesn't > include blink. Blink provides idle callback handling, memory pressure, > fast JS bindings, and more. Using v8 without blink is not good, and the > existing Gin bindings really need to get migrated. > > This callback out into the embedder that web UI is using is a hack, and > should be removed. Blink should have the low level mojo functions > Send(), etc. in actual blink idl files. Web UI should just include > generated JS files using http requests. This EnableMojoBindings thing > needs to go away. > It's great if we get benefits from using v8+Blink, and it's fine if Blink IDL + impl is is the only way in which Mojo + JS is ever exposed within Chrome, content, whatever. No problems with that. Getting rid of the EnableMojoBindings hack would be great. I do want to make sure it's clear though, we are not in agreement that a v8 dependency must imply a Blink dependency, and I'm not interested in changing Mojo such that it is ever strictly necessary to depend on Blink in order to use some JS bindings. > https://codereview.chromium.org/1707233003/ > -- 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.
On 2016/02/24 at 21:01:22, rockot wrote: > On Wed, Feb 24, 2016 at 12:44 PM, <esprehn@chromium.org> wrote: > > ... > > It's great if we get benefits from using v8+Blink, and it's fine if Blink > IDL + impl is is the only way in which Mojo + JS is ever exposed within > Chrome, content, whatever. No problems with that. Getting rid of the > EnableMojoBindings hack would be great. > > I do want to make sure it's clear though, we are not in agreement that a v8 > dependency must imply a Blink dependency, and I'm not interested in > changing Mojo such that it is ever strictly necessary to depend on Blink in > order to use some JS bindings. > Where are you going to use mojo with v8 and not have blink?
On Wed, Feb 24, 2016 at 1:59 PM, <esprehn@chromium.org> wrote: > On 2016/02/24 at 21:01:22, rockot wrote: > > On Wed, Feb 24, 2016 at 12:44 PM, <esprehn@chromium.org> wrote: > > > > ... > > > > It's great if we get benefits from using v8+Blink, and it's fine if Blink > > IDL + impl is is the only way in which Mojo + JS is ever exposed within > > Chrome, content, whatever. No problems with that. Getting rid of the > > EnableMojoBindings hack would be great. > > > > I do want to make sure it's clear though, we are not in agreement that a > v8 > > dependency must imply a Blink dependency, and I'm not interested in > > changing Mojo such that it is ever strictly necessary to depend on Blink > in > > order to use some JS bindings. > > > > Where are you going to use mojo with v8 and not have blink? > Not sure, but it's something we want to support. An example would be if we want to run lightweight system applications or services written in JS. It would not be reasonable to assume that every Mojo application will want or need anything Blink provides. > https://codereview.chromium.org/1707233003/ > -- 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.
On Wed, Feb 24, 2016 at 1:59 PM, <esprehn@chromium.org> wrote: > On 2016/02/24 at 21:01:22, rockot wrote: > > On Wed, Feb 24, 2016 at 12:44 PM, <esprehn@chromium.org> wrote: > > > > ... > > > > It's great if we get benefits from using v8+Blink, and it's fine if Blink > > IDL + impl is is the only way in which Mojo + JS is ever exposed within > > Chrome, content, whatever. No problems with that. Getting rid of the > > EnableMojoBindings hack would be great. > > > > I do want to make sure it's clear though, we are not in agreement that a > v8 > > dependency must imply a Blink dependency, and I'm not interested in > > changing Mojo such that it is ever strictly necessary to depend on Blink > in > > order to use some JS bindings. > > > > Where are you going to use mojo with v8 and not have blink? > Not sure, but it's something we want to support. An example would be if we want to run lightweight system applications or services written in JS. It would not be reasonable to assume that every Mojo application will want or need anything Blink provides. > https://codereview.chromium.org/1707233003/ > -- 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.
On 2016/02/24 at 22:07:33, rockot wrote: > On Wed, Feb 24, 2016 at 1:59 PM, <esprehn@chromium.org> wrote: > > > On 2016/02/24 at 21:01:22, rockot wrote: > > > On Wed, Feb 24, 2016 at 12:44 PM, <esprehn@chromium.org> wrote: > > > > > > ... > > > > > > It's great if we get benefits from using v8+Blink, and it's fine if Blink > > > IDL + impl is is the only way in which Mojo + JS is ever exposed within > > > Chrome, content, whatever. No problems with that. Getting rid of the > > > EnableMojoBindings hack would be great. > > > > > > I do want to make sure it's clear though, we are not in agreement that a > > v8 > > > dependency must imply a Blink dependency, and I'm not interested in > > > changing Mojo such that it is ever strictly necessary to depend on Blink > > in > > > order to use some JS bindings. > > > > > > > Where are you going to use mojo with v8 and not have blink? > > > > Not sure, but it's something we want to support. An example would be if we > want to run lightweight system applications or services written in JS. It > would not be reasonable to assume that every Mojo application will want or > need anything Blink provides. > That doesn't seem reasonable to me, who will notify v8 of memory pressure, or when to handle idle task GCs, will you reimplement the caches, security checks, and bindings APIs? Blink is the platform that provides a robust runtime for JS applications. Using v8 in Chrome without blink doesn't make sense. Blink is already in the child process and the utility process. Blink is not just the "web runtime" you can opt to not use, it provides much more than that.
On Wed, Feb 24, 2016 at 2:12 PM, <esprehn@chromium.org> wrote: > On 2016/02/24 at 22:07:33, rockot wrote: > > On Wed, Feb 24, 2016 at 1:59 PM, <esprehn@chromium.org> wrote: > > > > > On 2016/02/24 at 21:01:22, rockot wrote: > > > > On Wed, Feb 24, 2016 at 12:44 PM, <esprehn@chromium.org> wrote: > > > > > > > > ... > > > > > > > > It's great if we get benefits from using v8+Blink, and it's fine if > Blink > > > > IDL + impl is is the only way in which Mojo + JS is ever exposed > within > > > > Chrome, content, whatever. No problems with that. Getting rid of the > > > > EnableMojoBindings hack would be great. > > > > > > > > I do want to make sure it's clear though, we are not in agreement > that a > > > v8 > > > > dependency must imply a Blink dependency, and I'm not interested in > > > > changing Mojo such that it is ever strictly necessary to depend on > Blink > > > in > > > > order to use some JS bindings. > > > > > > > > > > Where are you going to use mojo with v8 and not have blink? > > > > > > > Not sure, but it's something we want to support. An example would be if > we > > want to run lightweight system applications or services written in JS. It > > would not be reasonable to assume that every Mojo application will want > or > > need anything Blink provides. > > > > That doesn't seem reasonable to me, who will notify v8 of memory pressure, > or > when to handle idle task GCs, will you reimplement the caches, security > checks, > and bindings APIs? > > Blink is the platform that provides a robust runtime for JS applications. > Using > v8 in Chrome without blink doesn't make sense. Blink is already in the > child > process and the utility process. Blink is not just the "web runtime" you > can opt > to not use, it provides much more than that. > We're talking about applications that will run outside of Chrome. > > https://codereview.chromium.org/1707233003/ > -- 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.
On Wed, Feb 24, 2016 at 2:12 PM, <esprehn@chromium.org> wrote: > On 2016/02/24 at 22:07:33, rockot wrote: > > On Wed, Feb 24, 2016 at 1:59 PM, <esprehn@chromium.org> wrote: > > > > > On 2016/02/24 at 21:01:22, rockot wrote: > > > > On Wed, Feb 24, 2016 at 12:44 PM, <esprehn@chromium.org> wrote: > > > > > > > > ... > > > > > > > > It's great if we get benefits from using v8+Blink, and it's fine if > Blink > > > > IDL + impl is is the only way in which Mojo + JS is ever exposed > within > > > > Chrome, content, whatever. No problems with that. Getting rid of the > > > > EnableMojoBindings hack would be great. > > > > > > > > I do want to make sure it's clear though, we are not in agreement > that a > > > v8 > > > > dependency must imply a Blink dependency, and I'm not interested in > > > > changing Mojo such that it is ever strictly necessary to depend on > Blink > > > in > > > > order to use some JS bindings. > > > > > > > > > > Where are you going to use mojo with v8 and not have blink? > > > > > > > Not sure, but it's something we want to support. An example would be if > we > > want to run lightweight system applications or services written in JS. It > > would not be reasonable to assume that every Mojo application will want > or > > need anything Blink provides. > > > > That doesn't seem reasonable to me, who will notify v8 of memory pressure, > or > when to handle idle task GCs, will you reimplement the caches, security > checks, > and bindings APIs? > > Blink is the platform that provides a robust runtime for JS applications. > Using > v8 in Chrome without blink doesn't make sense. Blink is already in the > child > process and the utility process. Blink is not just the "web runtime" you > can opt > to not use, it provides much more than that. > We're talking about applications that will run outside of Chrome. > > https://codereview.chromium.org/1707233003/ > -- 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.
On 2016/02/24 at 22:13:43, rockot wrote: > ... > > We're talking about applications that will run outside of Chrome. > That doesn't seem relevant here though, the mojo binding generator inside Chrome should assume you're using blink for the JS bindings. I don't really care what a mojom compiler does for JS outside Chrome. :)
On Wed, Feb 24, 2016 at 2:15 PM, <esprehn@chromium.org> wrote: > On 2016/02/24 at 22:13:43, rockot wrote: > > ... > > > > We're talking about applications that will run outside of Chrome. > > > > That doesn't seem relevant here though, the mojo binding generator inside > Chrome > should assume you're using blink for the JS bindings. I don't really care > what a > mojom compiler does for JS outside Chrome. :) > Indeed, I think we're on the same page then! Hopefully we can start work on real Blink bindings soon. > > https://codereview.chromium.org/1707233003/ > -- 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.
On Wed, Feb 24, 2016 at 2:15 PM, <esprehn@chromium.org> wrote: > On 2016/02/24 at 22:13:43, rockot wrote: > > ... > > > > We're talking about applications that will run outside of Chrome. > > > > That doesn't seem relevant here though, the mojo binding generator inside > Chrome > should assume you're using blink for the JS bindings. I don't really care > what a > mojom compiler does for JS outside Chrome. :) > Indeed, I think we're on the same page then! Hopefully we can start work on real Blink bindings soon. > > https://codereview.chromium.org/1707233003/ > -- 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.
On 2016/02/24 at 22:16:39, rockot wrote: > On Wed, Feb 24, 2016 at 2:15 PM, <esprehn@chromium.org> wrote: > > > On 2016/02/24 at 22:13:43, rockot wrote: > > > ... > > > > > > We're talking about applications that will run outside of Chrome. > > > > > > > That doesn't seem relevant here though, the mojo binding generator inside > > Chrome > > should assume you're using blink for the JS bindings. I don't really care > > what a > > mojom compiler does for JS outside Chrome. :) > > > > Indeed, I think we're on the same page then! Hopefully we can start work on > real Blink bindings soon. > Yay, sounds fantastic. :D
Charlie: is there a path forward on this? I do feel that the current mojo bindings code could use some refactoring: MojoContextState and friends cover two similar but not quite identical use cases, and then only one use case can be used per frame. However, it doesn't seem particularly worthwhile to fix it now, since nothing should be using both sorts of mojo bindings at the same time (we can document and DCHECK this) and it seems that the plan for these bindings is to move into Blink.
On 2016/03/11 04:16:20, Sam McNally wrote: > Charlie: is there a path forward on this? > > I do feel that the current mojo bindings code could use some refactoring: > MojoContextState and friends cover two similar but not quite identical use > cases, and then only one use case can be used per frame. However, it doesn't > seem particularly worthwhile to fix it now, since nothing should be using both > sorts of mojo bindings at the same time (we can document and DCHECK this) and it > seems that the plan for these bindings is to move into Blink. Sorry for not replying earlier-- I was under the impression that we were removing EnableMojoBindings and moving the bindings into Blink instead of doing this patch. I'd kind of prefer that strategy if possible, but I understand if something needs to be done in the meantime. If we move forward here in the meantime, I think there's a strong reason to make this behavior less subtle. Maybe we can punt on the WebUI vs layout test thing (which feels strange to me on its own) and just try to simplify the EnableMojoBindings calls. Could they all be made from RenderFrameImpl? Or at least be done in a way that more clearly indicates and/or asserts that WebUI and layout tests are mutually exclusive?
Patchset #3 (id:80001) has been deleted
On 2016/03/12 00:09:13, Charlie Reis (slow til 3-15) wrote: > On 2016/03/11 04:16:20, Sam McNally wrote: > > Charlie: is there a path forward on this? > > > > I do feel that the current mojo bindings code could use some refactoring: > > MojoContextState and friends cover two similar but not quite identical use > > cases, and then only one use case can be used per frame. However, it doesn't > > seem particularly worthwhile to fix it now, since nothing should be using both > > sorts of mojo bindings at the same time (we can document and DCHECK this) and > it > > seems that the plan for these bindings is to move into Blink. > > Sorry for not replying earlier-- I was under the impression that we were > removing EnableMojoBindings and moving the bindings into Blink instead of doing > this patch. I'd kind of prefer that strategy if possible, but I understand if > something needs to be done in the meantime. > > If we move forward here in the meantime, I think there's a strong reason to make > this behavior less subtle. Maybe we can punt on the WebUI vs layout test thing > (which feels strange to me on its own) and just try to simplify the > EnableMojoBindings calls. Could they all be made from RenderFrameImpl? Or at > least be done in a way that more clearly indicates and/or asserts that WebUI and > layout tests are mutually exclusive? Done.
On 2016/03/14 02:03:44, Sam McNally wrote: > On 2016/03/12 00:09:13, Charlie Reis (slow til 3-15) wrote: > > On 2016/03/11 04:16:20, Sam McNally wrote: > > > Charlie: is there a path forward on this? > > > > > > I do feel that the current mojo bindings code could use some refactoring: > > > MojoContextState and friends cover two similar but not quite identical use > > > cases, and then only one use case can be used per frame. However, it doesn't > > > seem particularly worthwhile to fix it now, since nothing should be using > both > > > sorts of mojo bindings at the same time (we can document and DCHECK this) > and > > it > > > seems that the plan for these bindings is to move into Blink. > > > > Sorry for not replying earlier-- I was under the impression that we were > > removing EnableMojoBindings and moving the bindings into Blink instead of > doing > > this patch. I'd kind of prefer that strategy if possible, but I understand if > > something needs to be done in the meantime. > > > > If we move forward here in the meantime, I think there's a strong reason to > make > > this behavior less subtle. Maybe we can punt on the WebUI vs layout test > thing > > (which feels strange to me on its own) and just try to simplify the > > EnableMojoBindings calls. Could they all be made from RenderFrameImpl? Or at > > least be done in a way that more clearly indicates and/or asserts that WebUI > and > > layout tests are mutually exclusive? > > Done. Thanks, that's much clearer than before. That should hold us over until the bindings are moved to Blink. content/ LGTM.
The CQ bit was checked by sammc@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org Link to the patchset: https://codereview.chromium.org/1707233003/#ps120001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1707233003/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1707233003/120001
Message was sent while issue was closed.
Committed patchset #4 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Expose mojo bindings to subframes in layout tests. ========== to ========== Expose mojo bindings to subframes in layout tests. Committed: https://crrev.com/e2f999841cbfba72585eef56746ab019c0db3b54 Cr-Commit-Position: refs/heads/master@{#381097} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/e2f999841cbfba72585eef56746ab019c0db3b54 Cr-Commit-Position: refs/heads/master@{#381097} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
