Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2531133002:
Convert SetIsIncognitoProcess to use mojo. (Closed)
|
Created:
4 years ago by nigeltao1 Modified:
4 years ago CC:
Aaron Boodman, abarth-chromium, chromium-reviews, darin (slow to review), darin-cc_chromium.org, jam, mlamouri+watch-content_chromium.org, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionConvert SetIsIncognitoProcess to use mojo.
Also add a hook for content::RenderThreadObserver implementations, such
as ChromeRenderThreadObserver, to register their Mojo interfaces.
Tested manually, by creating a new Chrome extension a la
https://developer.chrome.com/extensions/getstarted
and replacing its popup.js script with:
document.addEventListener('DOMContentLoaded', function() {
document.getElementById('status').textContent =
'inIncognitoContext: ' + chrome.extension.inIncognitoContext;
});
BUG=577685
Committed: https://crrev.com/7cd8d558f5731ea6da8de7334dacb5fecc04b15f
Cr-Commit-Position: refs/heads/master@{#437823}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Convert SetIsIncognitoProcess to use mojo. #
Total comments: 13
Patch Set 3 : Convert SetIsIncognitoProcess to use mojo. #
Total comments: 2
Patch Set 4 : Convert SetIsIncognitoProcess to use mojo. #
Total comments: 2
Patch Set 5 : Convert SetIsIncognitoProcess to use mojo. #Patch Set 6 : Convert SetIsIncognitoProcess to use mojo. #
Total comments: 5
Patch Set 7 : Convert SetIsIncognitoProcess to use mojo. #
Total comments: 2
Patch Set 8 : Convert SetIsIncognitoProcess to use mojo. #
Total comments: 2
Patch Set 9 : Convert SetIsIncognitoProcess to use mojo. #Patch Set 10 : Convert SetIsIncognitoProcess to use mojo. #Patch Set 11 : Convert SetIsIncognitoProcess to use mojo. #
Messages
Total messages: 67 (32 generated)
The CQ bit was checked by nigeltao@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...
nigeltao@chromium.org changed reviewers: + rockot@chromium.org
https://codereview.chromium.org/2531133002/diff/1/chrome/renderer/chrome_rend... File chrome/renderer/chrome_render_thread_observer.h (right): https://codereview.chromium.org/2531133002/diff/1/chrome/renderer/chrome_rend... chrome/renderer/chrome_render_thread_observer.h:38: public chrome::mojom::Renderer { Should this be NON_EXPORTED_BASE(public chrome::mojom::Renderer) I don't know (AFAICT it's a MSVC / Windows thing??), but I saw this in passing for the content::mojom::Renderer code that I copy/pasted.
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_...)
LGTM https://codereview.chromium.org/2531133002/diff/1/chrome/renderer/chrome_rend... File chrome/renderer/chrome_render_thread_observer.h (right): https://codereview.chromium.org/2531133002/diff/1/chrome/renderer/chrome_rend... chrome/renderer/chrome_render_thread_observer.h:38: public chrome::mojom::Renderer { On 2016/11/27 at 01:15:49, nigeltao1 wrote: > Should this be > > NON_EXPORTED_BASE(public chrome::mojom::Renderer) > > I don't know (AFAICT it's a MSVC / Windows thing??), but I saw this in passing for the content::mojom::Renderer code that I copy/pasted. That's only relevant if the inheriting class is exported, which this is not.
https://codereview.chromium.org/2531133002/diff/20001/chrome/renderer/chrome_... File chrome/renderer/chrome_render_thread_observer.cc (right): https://codereview.chromium.org/2531133002/diff/20001/chrome/renderer/chrome_... chrome/renderer/chrome_render_thread_observer.cc:280: base::Unretained(this))); BTW, I am not 100% confident that this is correct wrt lifetime issues. See also the new "TODO: unregister the mojo interfaces?" in content/renderer/render_thread_impl.cc. Can you please take another look at this? https://codereview.chromium.org/2531133002/diff/20001/content/renderer/render... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/2531133002/diff/20001/content/renderer/render... content/renderer/render_thread_impl.cc:1180: // TODO: unregister the mojo interfaces?? How? This TODO might be related. Either way, I'm also not 100% confident if there are any thread-safety or lifetime issues with calling InterfaceRegistry::RemoveInterface.
A few extra comments upon closer inspection. https://codereview.chromium.org/2531133002/diff/20001/chrome/common/renderer.... File chrome/common/renderer.mojom (right): https://codereview.chromium.org/2531133002/diff/20001/chrome/common/renderer.... chrome/common/renderer.mojom:23: InitialConfiguration(InitialConfigurationParams params); Please just pass a bool argument. You need need a Params struct for this. https://codereview.chromium.org/2531133002/diff/20001/content/common/renderer... File content/common/renderer.mojom (right): https://codereview.chromium.org/2531133002/diff/20001/content/common/renderer... content/common/renderer.mojom:138: // See also chrome.mojom.Renderer defined in chrome/common/renderer.mojom. Please omit this comment. We shouldn't be referring to chrome/ within content/, even for documentation. I'm sure there are extant violations of that rule, but let's not add more. https://codereview.chromium.org/2531133002/diff/20001/content/renderer/render... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/2531133002/diff/20001/content/renderer/render... content/renderer/render_thread_impl.cc:1180: // TODO: unregister the mojo interfaces?? How? On 2016/12/01 at 00:30:16, nigeltao1 wrote: > This TODO might be related. Either way, I'm also not 100% confident if there are any thread-safety or lifetime issues with calling InterfaceRegistry::RemoveInterface. Ah, missed this. First, as a general rule, unless a class is documented as thread-safe, it's not thread-safe. And if a class is not thread-safe, we already must take care to only use it from a single thread. This AssociatedInterfaceRegistry is therefore only used on one thread, the same thread which owns and exclusively calls into RenderThreadImpl (that's the main thread, FWIW). In fact it's trivial to see that the AssociatedInterfaceRegistry is directly owned by the RenderThreadImpl and therefore must still be valid if the RenderThreadImpl is still valid. I think it would be reasonable to add an RenderThreadObserver::UnregisterMojoInterfaces call which uses AssociatedInterfaceRegistry::RemoveInterface. It seems slightly unfortunate since it would be easy to forget to implement this, but there are relatively few RenderThreadObservers in the world, so I might feel worse about adding anything more complex.
https://codereview.chromium.org/2531133002/diff/20001/chrome/common/renderer.... File chrome/common/renderer.mojom (right): https://codereview.chromium.org/2531133002/diff/20001/chrome/common/renderer.... chrome/common/renderer.mojom:23: InitialConfiguration(InitialConfigurationParams params); On 2016/12/01 at 18:46:30, Ken Rockot wrote: > Please just pass a bool argument. You need need a Params struct for this. Err... that should be "don't need" https://codereview.chromium.org/2531133002/diff/20001/chrome/renderer/chrome_... File chrome/renderer/chrome_render_thread_observer.cc (right): https://codereview.chromium.org/2531133002/diff/20001/chrome/renderer/chrome_... chrome/renderer/chrome_render_thread_observer.cc:280: base::Unretained(this))); On 2016/12/01 at 00:30:16, nigeltao1 wrote: > BTW, I am not 100% confident that this is correct wrt lifetime issues. See also the new "TODO: unregister the mojo interfaces?" in content/renderer/render_thread_impl.cc. > > Can you please take another look at this? To clarify, in this case it is technically fine since the ChromeRenderThreadObserver is never removed and therefore lives as long as the RenderThreadImpl. But it would be good to explicitly unregister as mentioned in other comments, so that the correct usage pattern is demonstrated by this CL.
https://codereview.chromium.org/2531133002/diff/20001/chrome/common/renderer.... File chrome/common/renderer.mojom (right): https://codereview.chromium.org/2531133002/diff/20001/chrome/common/renderer.... chrome/common/renderer.mojom:23: InitialConfiguration(InitialConfigurationParams params); It could be just a bool in this CL, but as per the TODO above, I intend to add more fields to the params struct when I port the ChromeViewMsg_SetContentSettingRules legacy IPC message to Mojo. It's sent in ChromeContentBrowserClient::RenderProcessWillLaunch in chrome/browser/chrome_content_browser_client.cc just next to the ChromeViewMsg_SetIsIncognitoProcess legacy IPC message that I replaced in this CL.
https://codereview.chromium.org/2531133002/diff/20001/chrome/common/renderer.... File chrome/common/renderer.mojom (right): https://codereview.chromium.org/2531133002/diff/20001/chrome/common/renderer.... chrome/common/renderer.mojom:23: InitialConfiguration(InitialConfigurationParams params); On 2016/12/01 at 21:57:20, nigeltao1 wrote: > It could be just a bool in this CL, but as per the TODO above, I intend to add more fields to the params struct when I port the ChromeViewMsg_SetContentSettingRules legacy IPC message to Mojo. It's sent in ChromeContentBrowserClient::RenderProcessWillLaunch in chrome/browser/chrome_content_browser_client.cc just next to the ChromeViewMsg_SetIsIncognitoProcess legacy IPC message that I replaced in this CL. I would still then do: SetInitialConfiguration(bool is_incognito, ContentSettingRules rules) Also nit while I'm thinking about it again: Please try to use imperative verb phrases for message names. SetInitialConfiguration instead of InitialConfiguration.
PTAL. https://codereview.chromium.org/2531133002/diff/20001/chrome/common/renderer.... File chrome/common/renderer.mojom (right): https://codereview.chromium.org/2531133002/diff/20001/chrome/common/renderer.... chrome/common/renderer.mojom:23: InitialConfiguration(InitialConfigurationParams params); On 2016/12/01 22:02:59, Ken Rockot wrote: > On 2016/12/01 at 21:57:20, nigeltao1 wrote: > > It could be just a bool in this CL, but as per the TODO above, I intend to add > more fields to the params struct when I port the > ChromeViewMsg_SetContentSettingRules legacy IPC message to Mojo. It's sent in > ChromeContentBrowserClient::RenderProcessWillLaunch in > chrome/browser/chrome_content_browser_client.cc just next to the > ChromeViewMsg_SetIsIncognitoProcess legacy IPC message that I replaced in this > CL. > > I would still then do: > > SetInitialConfiguration(bool is_incognito, ContentSettingRules rules) > > Also nit while I'm thinking about it again: Please try to use imperative verb > phrases for message names. SetInitialConfiguration instead of > InitialConfiguration. Done. https://codereview.chromium.org/2531133002/diff/20001/chrome/renderer/chrome_... File chrome/renderer/chrome_render_thread_observer.cc (right): https://codereview.chromium.org/2531133002/diff/20001/chrome/renderer/chrome_... chrome/renderer/chrome_render_thread_observer.cc:280: base::Unretained(this))); On 2016/12/01 18:49:45, Ken Rockot wrote: > On 2016/12/01 at 00:30:16, nigeltao1 wrote: > > BTW, I am not 100% confident that this is correct wrt lifetime issues. See > also the new "TODO: unregister the mojo interfaces?" in > content/renderer/render_thread_impl.cc. > > > > Can you please take another look at this? > > To clarify, in this case it is technically fine since the > ChromeRenderThreadObserver is never removed and therefore lives as long as the > RenderThreadImpl. > > But it would be good to explicitly unregister as mentioned in other comments, so > that the correct usage pattern is demonstrated by this CL. Done. https://codereview.chromium.org/2531133002/diff/20001/content/common/renderer... File content/common/renderer.mojom (right): https://codereview.chromium.org/2531133002/diff/20001/content/common/renderer... content/common/renderer.mojom:138: // See also chrome.mojom.Renderer defined in chrome/common/renderer.mojom. On 2016/12/01 18:46:30, Ken Rockot wrote: > Please omit this comment. We shouldn't be referring to chrome/ within content/, > even for documentation. I'm sure there are extant violations of that rule, but > let's not add more. Done. https://codereview.chromium.org/2531133002/diff/20001/content/renderer/render... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/2531133002/diff/20001/content/renderer/render... content/renderer/render_thread_impl.cc:1180: // TODO: unregister the mojo interfaces?? How? On 2016/12/01 18:46:30, Ken Rockot wrote: > On 2016/12/01 at 00:30:16, nigeltao1 wrote: > > This TODO might be related. Either way, I'm also not 100% confident if there > are any thread-safety or lifetime issues with calling > InterfaceRegistry::RemoveInterface. > > Ah, missed this. > > First, as a general rule, unless a class is documented as thread-safe, it's not > thread-safe. And if a class is not thread-safe, we already must take care to > only use it from a single thread. This AssociatedInterfaceRegistry is therefore > only used on one thread, the same thread which owns and exclusively calls into > RenderThreadImpl (that's the main thread, FWIW). > > In fact it's trivial to see that the AssociatedInterfaceRegistry is directly > owned by the RenderThreadImpl and therefore must still be valid if the > RenderThreadImpl is still valid. > > I think it would be reasonable to add an > RenderThreadObserver::UnregisterMojoInterfaces call which uses > AssociatedInterfaceRegistry::RemoveInterface. It seems slightly unfortunate > since it would be easy to forget to implement this, but there are relatively few > RenderThreadObservers in the world, so I might feel worse about adding anything > more complex. Done, although just dropping some printf's here confirms that not every AddObserver is currently balanced by a RemoveObserver, so I haven't (manually or otherwise) tested the RemoveInterface codepath. As you already said, though, ChromeRenderThreadObserver is never removed, so I assume that it's OK.
The CQ bit was checked by nigeltao@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...
Thanks! LGTM for realsies. https://codereview.chromium.org/2531133002/diff/40001/chrome/browser/chrome_c... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2531133002/diff/40001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.cc:1094: GetGuestViewDefaultContentSettingRules(profile->IsOffTheRecord(), &rules); nit: might as well reuse is_incognito_process here if you're going to introduce it above.
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_...)
https://codereview.chromium.org/2531133002/diff/40001/chrome/browser/chrome_c... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2531133002/diff/40001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.cc:1094: GetGuestViewDefaultContentSettingRules(profile->IsOffTheRecord(), &rules); On 2016/12/02 05:50:41, Ken Rockot wrote: > nit: might as well reuse is_incognito_process here if you're going to introduce > it above. Done.
The CQ bit was checked by nigeltao@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/2531133002/#ps60001 (title: "Convert SetIsIncognitoProcess to use mojo.")
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...)
nigeltao@chromium.org changed reviewers: + sky@chromium.org, tsepez@chromium.org
R=sky,tsepez for OWNERS.
https://codereview.chromium.org/2531133002/diff/60001/chrome/common/renderer.... File chrome/common/renderer.mojom (right): https://codereview.chromium.org/2531133002/diff/60001/chrome/common/renderer.... chrome/common/renderer.mojom:10: interface Renderer { For an interface that is meant to contain control/configuration information this name is very generic. Maybe RendererConfiguration?
OWNERS LGTM, but address sky's comment. Thanks.
https://codereview.chromium.org/2531133002/diff/60001/chrome/common/renderer.... File chrome/common/renderer.mojom (right): https://codereview.chromium.org/2531133002/diff/60001/chrome/common/renderer.... chrome/common/renderer.mojom:10: interface Renderer { On 2016/12/05 16:11:22, sky wrote: > For an interface that is meant to contain control/configuration information this > name is very generic. Maybe RendererConfiguration? This (chrome.mojom.Renderer) is meant to be the s/content/chrome/ analog of the existing content.mojom.Renderer interface. As rockot said in https://groups.google.com/a/chromium.org/d/msg/chromium-mojo/sGmLgv-H28w/jx06... "For one example there's the Renderer Channel-associated interface -- this is just a bag of miscellaneous browser-to-renderer messages which have been converted. It's accessible via RenderProcessHost::GetRendererInterface and is safe to call into very early. You could mimic this approach for messages like SetIsIncognitoProcess, but since it's in the src/chrome layer you'll need to introduce a new interface and bind it somewhere in the renderer". That's exactly what I did.
On Wed, Dec 7, 2016 at 8:33 PM, <nigeltao@chromium.org> wrote: > > https://codereview.chromium.org/2531133002/diff/60001/ > chrome/common/renderer.mojom > File chrome/common/renderer.mojom (right): > > https://codereview.chromium.org/2531133002/diff/60001/ > chrome/common/renderer.mojom#newcode10 > chrome/common/renderer.mojom:10: interface Renderer { > On 2016/12/05 16:11:22, sky wrote: > > For an interface that is meant to contain control/configuration > information this > > name is very generic. Maybe RendererConfiguration? > > This (chrome.mojom.Renderer) is meant to be the s/content/chrome/ analog > of the existing content.mojom.Renderer interface. As rockot said in > https://groups.google.com/a/chromium.org/d/msg/chromium- > mojo/sGmLgv-H28w/jx06gT85AAAJ > > "For one example there's the Renderer Channel-associated interface -- > this is just a bag of miscellaneous browser-to-renderer messages which > have been converted. It's accessible via > RenderProcessHost::GetRendererInterface and is safe to call into very > early. > > You could mimic this approach for messages like SetIsIncognitoProcess, > but since it's in the src/chrome layer you'll need to introduce a new > interface and bind it somewhere in the renderer". > > That's exactly what I did. > Sure, it's reasonable, but I think sky's comment is reasonable too. If the interface is only going to be used for this singular IPC in practice, it might make sense to give it a more specific name. content's Renderer mojom was a conversion of all messages handled directly by RenderThreadImpl, and unfortunately there was no common theme among them. "Renderer" was just my cop-out name for it. > > https://codereview.chromium.org/2531133002/ > -- 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/12/08 04:42:20, Ken Rockot wrote: > Sure, it's reasonable, but I think sky's comment is reasonable too. If the > interface is only going to be used for this singular IPC in practice, it > might make sense to give it a more specific name. OK. Done.
https://codereview.chromium.org/2531133002/diff/80002/chrome/renderer/chrome_... File chrome/renderer/chrome_render_thread_observer.cc (right): https://codereview.chromium.org/2531133002/diff/80002/chrome/renderer/chrome_... chrome/renderer/chrome_render_thread_observer.cc:315: if (renderer_configuration_binding_.is_bound()) rockot, please take a close look at this.
The CQ bit was checked by nigeltao@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 unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
LGTM https://codereview.chromium.org/2531133002/diff/80002/chrome/renderer/chrome_... File chrome/renderer/chrome_render_thread_observer.cc (right): https://codereview.chromium.org/2531133002/diff/80002/chrome/renderer/chrome_... chrome/renderer/chrome_render_thread_observer.cc:291: void ChromeRenderThreadObserver::OnRendererInterfaceRequest( Make declaration/definition order match. https://codereview.chromium.org/2531133002/diff/80002/chrome/renderer/chrome_... chrome/renderer/chrome_render_thread_observer.cc:314: // https://groups.google.com/a/chromium.org/d/msg/chromium-mojo/BKjxN1DLdrc/vBCU... I would file a bug on this.
https://codereview.chromium.org/2531133002/diff/80002/chrome/renderer/chrome_... File chrome/renderer/chrome_render_thread_observer.cc (right): https://codereview.chromium.org/2531133002/diff/80002/chrome/renderer/chrome_... chrome/renderer/chrome_render_thread_observer.cc:291: void ChromeRenderThreadObserver::OnRendererInterfaceRequest( On 2016/12/08 16:06:23, sky wrote: > Make declaration/definition order match. Ha, I discovered some unused declarations in the .h file. I sent out https://codereview.chromium.org/2558993004 Done. https://codereview.chromium.org/2531133002/diff/80002/chrome/renderer/chrome_... chrome/renderer/chrome_render_thread_observer.cc:314: // https://groups.google.com/a/chromium.org/d/msg/chromium-mojo/BKjxN1DLdrc/vBCU... On 2016/12/08 16:06:24, sky wrote: > I would file a bug on this. I filed http://crbug.com/672646
lgtm https://codereview.chromium.org/2531133002/diff/110001/chrome/renderer/chrome... File chrome/renderer/chrome_render_thread_observer.cc (right): https://codereview.chromium.org/2531133002/diff/110001/chrome/renderer/chrome... chrome/renderer/chrome_render_thread_observer.cc:305: // This renderer_configuration_binding_.Unbind call works around tests that nit: Probably sufficient to just way "Workaround for http://crbug.com/672646"
https://codereview.chromium.org/2531133002/diff/110001/chrome/renderer/chrome... File chrome/renderer/chrome_render_thread_observer.cc (right): https://codereview.chromium.org/2531133002/diff/110001/chrome/renderer/chrome... chrome/renderer/chrome_render_thread_observer.cc:305: // This renderer_configuration_binding_.Unbind call works around tests that On 2016/12/09 04:29:08, Ken Rockot wrote: > nit: Probably sufficient to just way "Workaround for http://crbug.com/672646%22 Done.
The CQ bit was checked by nigeltao@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org, sky@chromium.org, rockot@chromium.org Link to the patchset: https://codereview.chromium.org/2531133002/#ps130001 (title: "Convert SetIsIncognitoProcess to use mojo.")
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...)
nigeltao@chromium.org changed reviewers: + jochen@chromium.org
jochen, can you review for content/OWNERS?
lgtm with comment addressed https://codereview.chromium.org/2531133002/diff/130001/content/public/rendere... File content/public/renderer/render_thread_observer.h (right): https://codereview.chromium.org/2531133002/diff/130001/content/public/rendere... content/public/renderer/render_thread_observer.h:10: #include "content/public/common/associated_interface_registry.h" is this needed? Should be enough to forward declare AssociatedInterfaceRegistry, no?
https://codereview.chromium.org/2531133002/diff/130001/content/public/rendere... File content/public/renderer/render_thread_observer.h (right): https://codereview.chromium.org/2531133002/diff/130001/content/public/rendere... content/public/renderer/render_thread_observer.h:10: #include "content/public/common/associated_interface_registry.h" On 2016/12/09 08:28:26, jochen wrote: > is this needed? Should be enough to forward declare AssociatedInterfaceRegistry, > no? Done.
The CQ bit was checked by nigeltao@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org, rockot@chromium.org, jochen@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2531133002/#ps150001 (title: "Convert SetIsIncognitoProcess to use mojo.")
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: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
The CQ bit was checked by nigeltao@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org, rockot@chromium.org, jochen@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2531133002/#ps170001 (title: "Convert SetIsIncognitoProcess to use mojo.")
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: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by nigeltao@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org, rockot@chromium.org, jochen@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2531133002/#ps190001 (title: "Convert SetIsIncognitoProcess to use mojo.")
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": 190001, "attempt_start_ts": 1481517048659670,
"parent_rev": "7d0e7f90f6fa5d908239105e89b0bbe35b4842a9", "commit_rev":
"3878643b7da3ed796032b3043d20c7608fe93a9e"}
Message was sent while issue was closed.
Description was changed from ========== Convert SetIsIncognitoProcess to use mojo. Also add a hook for content::RenderThreadObserver implementations, such as ChromeRenderThreadObserver, to register their Mojo interfaces. Tested manually, by creating a new Chrome extension a la https://developer.chrome.com/extensions/getstarted and replacing its popup.js script with: document.addEventListener('DOMContentLoaded', function() { document.getElementById('status').textContent = 'inIncognitoContext: ' + chrome.extension.inIncognitoContext; }); BUG=577685 ========== to ========== Convert SetIsIncognitoProcess to use mojo. Also add a hook for content::RenderThreadObserver implementations, such as ChromeRenderThreadObserver, to register their Mojo interfaces. Tested manually, by creating a new Chrome extension a la https://developer.chrome.com/extensions/getstarted and replacing its popup.js script with: document.addEventListener('DOMContentLoaded', function() { document.getElementById('status').textContent = 'inIncognitoContext: ' + chrome.extension.inIncognitoContext; }); BUG=577685 Review-Url: https://codereview.chromium.org/2531133002 ==========
Message was sent while issue was closed.
Committed patchset #11 (id:190001)
Message was sent while issue was closed.
Description was changed from ========== Convert SetIsIncognitoProcess to use mojo. Also add a hook for content::RenderThreadObserver implementations, such as ChromeRenderThreadObserver, to register their Mojo interfaces. Tested manually, by creating a new Chrome extension a la https://developer.chrome.com/extensions/getstarted and replacing its popup.js script with: document.addEventListener('DOMContentLoaded', function() { document.getElementById('status').textContent = 'inIncognitoContext: ' + chrome.extension.inIncognitoContext; }); BUG=577685 Review-Url: https://codereview.chromium.org/2531133002 ========== to ========== Convert SetIsIncognitoProcess to use mojo. Also add a hook for content::RenderThreadObserver implementations, such as ChromeRenderThreadObserver, to register their Mojo interfaces. Tested manually, by creating a new Chrome extension a la https://developer.chrome.com/extensions/getstarted and replacing its popup.js script with: document.addEventListener('DOMContentLoaded', function() { document.getElementById('status').textContent = 'inIncognitoContext: ' + chrome.extension.inIncognitoContext; }); BUG=577685 Committed: https://crrev.com/7cd8d558f5731ea6da8de7334dacb5fecc04b15f Cr-Commit-Position: refs/heads/master@{#437823} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/7cd8d558f5731ea6da8de7334dacb5fecc04b15f Cr-Commit-Position: refs/heads/master@{#437823} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
