Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2218843003:
mustash chrome: Change how mus connection is set up in chrome. (Closed)
Descriptionmustash chrome: Change how mus connection is set up in chrome.
Set up the mus connection (through WindowManagerConnection) in chrome
a little earlier than before, so that its mus gpu service can be set up
and injected into content cleanly (see more in crrev.com/2197613003).
To make it possible without introducing additional API in content, move
the WindowManagerConnection ownership into ChromeContentBrowserClient
from ChromeBrowserMainExtraPartsViews.
BUG=none
Patch Set 1 #Patch Set 2 : . #
Total comments: 3
Messages
Total messages: 18 (9 generated)
The CQ bit was checked by sadrul@chromium.org 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 checked by sadrul@chromium.org 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...
Description was changed from ========== mustash chrome: Change how mus connection is created in chrome. ... BUG=none ========== to ========== mustash chrome: Change how mus connection is set up in chrome. Set up the mus connection (through WindowManagerConnection) in chrome a little earlier than before, so that its mus gpu service can be set up and injected into content cleanly (see more in crrev.com/2197613003). To make it possible without introducing additional API in content, move the WindowManagerConnection ownership into ChromeContentBrowserClient from ChromeBrowserMainExtraPartsViews. BUG=none ==========
sadrul@chromium.org changed reviewers: + jam@chromium.org
Following up from our offline chat about https://codereview.chromium.org/2215353004/, this is the alternate solution I had in mind. PTAL. Thanks!
ben@chromium.org changed reviewers: + ben@chromium.org
https://codereview.chromium.org/2218843003/diff/2/chrome/browser/chrome_conte... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2218843003/diff/2/chrome/browser/chrome_conte... chrome/browser/chrome_content_browser_client.cc:2871: #if defined(USE_AURA) this seems like a really arbitrary place to do this initialization. what was the objection to having the specific method? fwiw, I have it on my list to figure out a more generic way of expressing what this method is actually designed for.
https://codereview.chromium.org/2218843003/diff/2/chrome/browser/chrome_conte... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2218843003/diff/2/chrome/browser/chrome_conte... chrome/browser/chrome_content_browser_client.cc:2871: #if defined(USE_AURA) On 2016/08/05 23:01:21, Ben Goodger (Google) wrote: > this seems like a really arbitrary place to do this initialization. > > what was the objection to having the specific method? I was asking Sadrul if there are ways of doing this without adding extra embedder callbacks from content. Given that we're trying to reduce content, having more intertwining seemed going in the other direction. btw we could keep this code in chrome_browser_main_extra_parts_views still right Sadurl? Perhaps just add RegisterOutOfProcessMojoApplications to ChromeBrowserMainExtraParts > > fwiw, I have it on my list to figure out a more generic way of expressing what > this method is actually designed for.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
https://codereview.chromium.org/2218843003/diff/2/chrome/browser/chrome_conte... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2218843003/diff/2/chrome/browser/chrome_conte... chrome/browser/chrome_content_browser_client.cc:2871: #if defined(USE_AURA) On 2016/08/05 23:07:07, jam wrote: > On 2016/08/05 23:01:21, Ben Goodger (Google) wrote: > > this seems like a really arbitrary place to do this initialization. > > > > what was the objection to having the specific method? > > I was asking Sadrul if there are ways of doing this without adding extra > embedder callbacks from content. Given that we're trying to reduce content, > having more intertwining seemed going in the other direction. > > btw we could keep this code in chrome_browser_main_extra_parts_views still right > Sadurl? Perhaps just add RegisterOutOfProcessMojoApplications to > ChromeBrowserMainExtraParts ChromeContentBrowserClient does create ChromeBrowserMainExtraPartsViews, but it passes ownership of that to content::BrowserMainLoop, and doesn't retain any reference to it. Keeping the WindowManagerConnection in the *ExtraPartsView, and triggering RegisterOutOfProcessMojoApplications() on the *MainExtraParts from here would be possible, but would not be very clean. (ChromeContentBrowserClient here would need to retain a raw pointer to the ChromeBrowserMainParts it creates in CreateBrowserMainParts() above, expose the *ExtraParts from ChromeBrowserMainParts and trigger the callback on them here (or add to the ChromeBrowserMainParts API to trigger the callback on the extra parts). This also makes the assumption (and a reasonably safe one, I suppose) that the ChromeBrowserMainParts object it created is still alive when we reach here. > > > > > fwiw, I have it on my list to figure out a more generic way of expressing what > > this method is actually designed for. >
On 2016/08/05 23:24:39, sadrul wrote: > https://codereview.chromium.org/2218843003/diff/2/chrome/browser/chrome_conte... > File chrome/browser/chrome_content_browser_client.cc (right): > > https://codereview.chromium.org/2218843003/diff/2/chrome/browser/chrome_conte... > chrome/browser/chrome_content_browser_client.cc:2871: #if defined(USE_AURA) > On 2016/08/05 23:07:07, jam wrote: > > On 2016/08/05 23:01:21, Ben Goodger (Google) wrote: > > > this seems like a really arbitrary place to do this initialization. > > > > > > what was the objection to having the specific method? > > > > I was asking Sadrul if there are ways of doing this without adding extra > > embedder callbacks from content. Given that we're trying to reduce content, > > having more intertwining seemed going in the other direction. > > > > btw we could keep this code in chrome_browser_main_extra_parts_views still > right > > Sadurl? Perhaps just add RegisterOutOfProcessMojoApplications to > > ChromeBrowserMainExtraParts > > ChromeContentBrowserClient does create ChromeBrowserMainExtraPartsViews, but it > passes ownership of that to content::BrowserMainLoop, and doesn't retain any > reference to it. Keeping the WindowManagerConnection in the *ExtraPartsView, and > triggering RegisterOutOfProcessMojoApplications() on the *MainExtraParts from > here would be possible, but would not be very clean. (ChromeContentBrowserClient > here would need to retain a raw pointer to the ChromeBrowserMainParts it creates > in CreateBrowserMainParts() above, expose the *ExtraParts from > ChromeBrowserMainParts and trigger the callback on them here (or add to the > ChromeBrowserMainParts API to trigger the callback on the extra parts). This > also makes the assumption (and a reasonably safe one, I suppose) that the > ChromeBrowserMainParts object it created is still alive when we reach here. i see, ok nvm
On 2016/08/08 16:53:58, jam wrote: > On 2016/08/05 23:24:39, sadrul wrote: > > > https://codereview.chromium.org/2218843003/diff/2/chrome/browser/chrome_conte... > > File chrome/browser/chrome_content_browser_client.cc (right): > > > > > https://codereview.chromium.org/2218843003/diff/2/chrome/browser/chrome_conte... > > chrome/browser/chrome_content_browser_client.cc:2871: #if defined(USE_AURA) > > On 2016/08/05 23:07:07, jam wrote: > > > On 2016/08/05 23:01:21, Ben Goodger (Google) wrote: > > > > this seems like a really arbitrary place to do this initialization. > > > > > > > > what was the objection to having the specific method? > > > > > > I was asking Sadrul if there are ways of doing this without adding extra > > > embedder callbacks from content. Given that we're trying to reduce content, > > > having more intertwining seemed going in the other direction. > > > > > > btw we could keep this code in chrome_browser_main_extra_parts_views still > > right > > > Sadurl? Perhaps just add RegisterOutOfProcessMojoApplications to > > > ChromeBrowserMainExtraParts > > > > ChromeContentBrowserClient does create ChromeBrowserMainExtraPartsViews, but > it > > passes ownership of that to content::BrowserMainLoop, and doesn't retain any > > reference to it. Keeping the WindowManagerConnection in the *ExtraPartsView, > and > > triggering RegisterOutOfProcessMojoApplications() on the *MainExtraParts from > > here would be possible, but would not be very clean. > (ChromeContentBrowserClient > > here would need to retain a raw pointer to the ChromeBrowserMainParts it > creates > > in CreateBrowserMainParts() above, expose the *ExtraParts from > > ChromeBrowserMainParts and trigger the callback on them here (or add to the > > ChromeBrowserMainParts API to trigger the callback on the extra parts). This > > also makes the assumption (and a reasonably safe one, I suppose) that the > > ChromeBrowserMainParts object it created is still alive when we reach here. > > i see, ok nvm ok cool, thanks. I will go ahead and put https://codereview.chromium.org/2215353004/ in the CQ.
Message was sent while issue was closed.
On 2016/08/08 17:11:20, sadrul wrote: > On 2016/08/08 16:53:58, jam wrote: > > On 2016/08/05 23:24:39, sadrul wrote: > > > > > > https://codereview.chromium.org/2218843003/diff/2/chrome/browser/chrome_conte... > > > File chrome/browser/chrome_content_browser_client.cc (right): > > > > > > > > > https://codereview.chromium.org/2218843003/diff/2/chrome/browser/chrome_conte... > > > chrome/browser/chrome_content_browser_client.cc:2871: #if defined(USE_AURA) > > > On 2016/08/05 23:07:07, jam wrote: > > > > On 2016/08/05 23:01:21, Ben Goodger (Google) wrote: > > > > > this seems like a really arbitrary place to do this initialization. > > > > > > > > > > what was the objection to having the specific method? > > > > > > > > I was asking Sadrul if there are ways of doing this without adding extra > > > > embedder callbacks from content. Given that we're trying to reduce > content, > > > > having more intertwining seemed going in the other direction. > > > > > > > > btw we could keep this code in chrome_browser_main_extra_parts_views still > > > right > > > > Sadurl? Perhaps just add RegisterOutOfProcessMojoApplications to > > > > ChromeBrowserMainExtraParts > > > > > > ChromeContentBrowserClient does create ChromeBrowserMainExtraPartsViews, but > > it > > > passes ownership of that to content::BrowserMainLoop, and doesn't retain any > > > reference to it. Keeping the WindowManagerConnection in the *ExtraPartsView, > > and > > > triggering RegisterOutOfProcessMojoApplications() on the *MainExtraParts > from > > > here would be possible, but would not be very clean. > > (ChromeContentBrowserClient > > > here would need to retain a raw pointer to the ChromeBrowserMainParts it > > creates > > > in CreateBrowserMainParts() above, expose the *ExtraParts from > > > ChromeBrowserMainParts and trigger the callback on them here (or add to the > > > ChromeBrowserMainParts API to trigger the callback on the extra parts). This > > > also makes the assumption (and a reasonably safe one, I suppose) that the > > > ChromeBrowserMainParts object it created is still alive when we reach here. > > > > i see, ok nvm > > ok cool, thanks. I will go ahead and put > https://codereview.chromium.org/2215353004/ in the CQ. oh what I meant is that this patch is good and don't need to change per my comment above. I think this is preferable to 2215353004 since it doesn't introduce new embedder APIs. Why do you prefer the other one?
On 2016/08/09 00:24:14, jam wrote: > On 2016/08/08 17:11:20, sadrul wrote: > > On 2016/08/08 16:53:58, jam wrote: > > > On 2016/08/05 23:24:39, sadrul wrote: > > > > > > > > > > https://codereview.chromium.org/2218843003/diff/2/chrome/browser/chrome_conte... > > > > File chrome/browser/chrome_content_browser_client.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2218843003/diff/2/chrome/browser/chrome_conte... > > > > chrome/browser/chrome_content_browser_client.cc:2871: #if > defined(USE_AURA) > > > > On 2016/08/05 23:07:07, jam wrote: > > > > > On 2016/08/05 23:01:21, Ben Goodger (Google) wrote: > > > > > > this seems like a really arbitrary place to do this initialization. > > > > > > > > > > > > what was the objection to having the specific method? > > > > > > > > > > I was asking Sadrul if there are ways of doing this without adding extra > > > > > embedder callbacks from content. Given that we're trying to reduce > > content, > > > > > having more intertwining seemed going in the other direction. > > > > > > > > > > btw we could keep this code in chrome_browser_main_extra_parts_views > still > > > > right > > > > > Sadurl? Perhaps just add RegisterOutOfProcessMojoApplications to > > > > > ChromeBrowserMainExtraParts > > > > > > > > ChromeContentBrowserClient does create ChromeBrowserMainExtraPartsViews, > but > > > it > > > > passes ownership of that to content::BrowserMainLoop, and doesn't retain > any > > > > reference to it. Keeping the WindowManagerConnection in the > *ExtraPartsView, > > > and > > > > triggering RegisterOutOfProcessMojoApplications() on the *MainExtraParts > > from > > > > here would be possible, but would not be very clean. > > > (ChromeContentBrowserClient > > > > here would need to retain a raw pointer to the ChromeBrowserMainParts it > > > creates > > > > in CreateBrowserMainParts() above, expose the *ExtraParts from > > > > ChromeBrowserMainParts and trigger the callback on them here (or add to > the > > > > ChromeBrowserMainParts API to trigger the callback on the extra parts). > This > > > > also makes the assumption (and a reasonably safe one, I suppose) that the > > > > ChromeBrowserMainParts object it created is still alive when we reach > here. > > > > > > i see, ok nvm > > > > ok cool, thanks. I will go ahead and put > > https://codereview.chromium.org/2215353004/ in the CQ. > > oh what I meant is that this patch is good and don't need to change per my > comment above. Oh shoot, sorry. I misunderstood. > > I think this is preferable to 2215353004 since it doesn't introduce new embedder > APIs. Why do you prefer the other one? I prefer the other one because, although it does add to the embedder API, it makes it more obvious that that particular step needs to happen after the shell connection has been established. Doing this in RegisterOutOfProcessMojoApplications() feels a bit random in comparison. For example, we can't set things up in RegisterInProcessMojoApplications(), since the shell connection has actually not been set up when that is called, but it's not obvious that that is the case without going in and looking at the actual code for MojoShellContext. (Similarly, we could just as well have set up the mus connection from the RegisterUnsandboxedOutOfProcessMojoApplications() override). I don't feel too strongly about this, either way. I can revert the landed CL and move forward with this one instead. I guess that would be your advice?
On 2016/08/09 00:42:31, sadrul wrote: > On 2016/08/09 00:24:14, jam wrote: > > On 2016/08/08 17:11:20, sadrul wrote: > > > On 2016/08/08 16:53:58, jam wrote: > > > > On 2016/08/05 23:24:39, sadrul wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/2218843003/diff/2/chrome/browser/chrome_conte... > > > > > File chrome/browser/chrome_content_browser_client.cc (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2218843003/diff/2/chrome/browser/chrome_conte... > > > > > chrome/browser/chrome_content_browser_client.cc:2871: #if > > defined(USE_AURA) > > > > > On 2016/08/05 23:07:07, jam wrote: > > > > > > On 2016/08/05 23:01:21, Ben Goodger (Google) wrote: > > > > > > > this seems like a really arbitrary place to do this initialization. > > > > > > > > > > > > > > what was the objection to having the specific method? > > > > > > > > > > > > I was asking Sadrul if there are ways of doing this without adding > extra > > > > > > embedder callbacks from content. Given that we're trying to reduce > > > content, > > > > > > having more intertwining seemed going in the other direction. > > > > > > > > > > > > btw we could keep this code in chrome_browser_main_extra_parts_views > > still > > > > > right > > > > > > Sadurl? Perhaps just add RegisterOutOfProcessMojoApplications to > > > > > > ChromeBrowserMainExtraParts > > > > > > > > > > ChromeContentBrowserClient does create ChromeBrowserMainExtraPartsViews, > > but > > > > it > > > > > passes ownership of that to content::BrowserMainLoop, and doesn't retain > > any > > > > > reference to it. Keeping the WindowManagerConnection in the > > *ExtraPartsView, > > > > and > > > > > triggering RegisterOutOfProcessMojoApplications() on the *MainExtraParts > > > from > > > > > here would be possible, but would not be very clean. > > > > (ChromeContentBrowserClient > > > > > here would need to retain a raw pointer to the ChromeBrowserMainParts it > > > > creates > > > > > in CreateBrowserMainParts() above, expose the *ExtraParts from > > > > > ChromeBrowserMainParts and trigger the callback on them here (or add to > > the > > > > > ChromeBrowserMainParts API to trigger the callback on the extra parts). > > This > > > > > also makes the assumption (and a reasonably safe one, I suppose) that > the > > > > > ChromeBrowserMainParts object it created is still alive when we reach > > here. > > > > > > > > i see, ok nvm > > > > > > ok cool, thanks. I will go ahead and put > > > https://codereview.chromium.org/2215353004/ in the CQ. > > > > oh what I meant is that this patch is good and don't need to change per my > > comment above. > > Oh shoot, sorry. I misunderstood. > > > > > I think this is preferable to 2215353004 since it doesn't introduce new > embedder > > APIs. Why do you prefer the other one? > > I prefer the other one because, although it does add to the embedder API, it > makes it more obvious that that particular step needs to happen after the shell > connection has been established. Doing this in > RegisterOutOfProcessMojoApplications() feels a bit random in comparison. For > example, we can't set things up in RegisterInProcessMojoApplications(), since > the shell connection has actually not been set up when that is called, but it's > not obvious that that is the case without going in and looking at the actual > code for MojoShellContext. (Similarly, we could just as well have set up the mus > connection from the RegisterUnsandboxedOutOfProcessMojoApplications() override). thanks for the explanation, that seems like a good reason to add the new api. > > I don't feel too strongly about this, either way. I can revert the landed CL and > move forward with this one instead. I guess that would be your advice? |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
