Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
409883006:
GCM: D-Bus methods for wake-on-packet (Closed)
|
Created:
6 years, 5 months ago by Luigi Semenzato Modified:
6 years, 3 months ago CC:
Chirantan Ekbote, chromium-reviews, oshima+watch_chromium.org, snanda_chromium.ort, stevenjb+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionGCM: D-Bus methods for wake-on-packet
Program the NIC to wake-on-packet for packets arriving from
the GCM server. Chrome notifies the connection manager (shill)
by calling D-Bus methods. The return status indicates if the
operation succeeded or not (depending on hardware support and
the type of connection).
Keywords: wowlan, wake-on-lan, wake-on-wlan
BUG=360295
TEST=verified dbus methods are called, and packets are sent on the
TEST=connection passed to the methods. Also verified that shill
TEST=programs the NIC correctly, and packets wake up the device.
Committed: https://crrev.com/88a95a70fa94bb578b371490d0f9868584b7096e
Cr-Commit-Position: refs/heads/master@{#294936}
Patch Set 1 #
Total comments: 6
Patch Set 2 : GCM: D-Bus methods for wake-on-packet #
Total comments: 3
Patch Set 3 : create a GCM driver early and attach app handler to it #Patch Set 4 : move chromeos-specific code to gcm profile creation #
Total comments: 20
Patch Set 5 : refining based on review comments #
Total comments: 8
Patch Set 6 : cleanup #
Total comments: 12
Patch Set 7 : fixes from review, plus change to dbus protocol #
Total comments: 1
Patch Set 8 : small fix, plus system_api DEPS #Patch Set 9 : remember to register the observer #
Total comments: 6
Patch Set 10 : GCM: D-Bus methods for wake-on-packet #Patch Set 11 : fix unit tests #
Total comments: 2
Patch Set 12 : fix unit tests really #Patch Set 13 : fix extension_gcm_app_handler_unittests #
Total comments: 3
Patch Set 14 : remove (void) casts #Patch Set 15 : don't break windows build #Messages
Total messages: 76 (12 generated)
NOTE: THIS CODE DOES NOT PASS THE PRE-SUBMIT CHECKS I think this is because the #include "chromeos/..." are not allowed in components/. In fact I tried to use a different approach but I haven't found a way, so I am uploading this code to clarify my goal. (The code is functionally correct.) I think the correct approach would be to offer a callback registration mechanism which would be used by ChromeOS-specific code (most likely in chrome/browser/chromeos). The appropriate callback would then be invoked at each GCM connection event (making a new connection and closing an existing connection). The problem is WHEN and HOW the callbacks can be registered. They must be registered through some GCM object, which maybe could be a ChromeOSAppHandler. But I haven't found a way of getting to such object from the ChromeOS initialization code. AFAICT, various GCM objects are created on demand in the context of a Profile. There is a static method for getting a list of all profiles, but it is deprecated. (Also I am not sure when the list would be populated with at least one profile.) Any advice on how to structure this will be greatly appreciated!
check with the gcm_driver owners to see if it's okay for it to depend on chromeos/ -- the top-level chromeos directory provides operating-system level constructs that are usually defined in system headers on other platforms; it's okay for most other parts of chrome to depend on it. if you can't do that, you can probably: a) define an interface under components/gcm_driver for registering wakeups with the OS b) create an implementation somewhere under chrome/browser/chromeos that talks to chromeos/ c) pass that early on into whatever creates GCM objects https://codereview.chromium.org/409883006/diff/1/chromeos/dbus/shill_manager_... File chromeos/dbus/shill_manager_client.h (right): https://codereview.chromium.org/409883006/diff/1/chromeos/dbus/shill_manager_... chromeos/dbus/shill_manager_client.h:227: const std::string& ip_connection, this should use net/base/ip_endpoint.h instead of an undocumented string; ditto for the other methods https://codereview.chromium.org/409883006/diff/1/components/gcm_driver/defaul... File components/gcm_driver/default_gcm_app_handler.cc (right): https://codereview.chromium.org/409883006/diff/1/components/gcm_driver/defaul... components/gcm_driver/default_gcm_app_handler.cc:56: LOG(ERROR) << "GCM connected to " << ip_endpoint_string; don't forget to change this back https://codereview.chromium.org/409883006/diff/1/components/gcm_driver/defaul... components/gcm_driver/default_gcm_app_handler.cc:67: LOG(ERROR) << "GCM disconnected"; or this
On 2014/07/22 20:32:01, Daniel Erat wrote: > check with the gcm_driver owners to see if it's okay for it to depend on > chromeos/ -- the top-level chromeos directory provides operating-system level > constructs that are usually defined in system headers on other platforms; it's > okay for most other parts of chrome to depend on it. > > if you can't do that, you can probably: > > a) define an interface under components/gcm_driver for registering wakeups with > the OS > b) create an implementation somewhere under chrome/browser/chromeos that talks > to chromeos/ > c) pass that early on into whatever creates GCM objects > > https://codereview.chromium.org/409883006/diff/1/chromeos/dbus/shill_manager_... > File chromeos/dbus/shill_manager_client.h (right): > > https://codereview.chromium.org/409883006/diff/1/chromeos/dbus/shill_manager_... > chromeos/dbus/shill_manager_client.h:227: const std::string& ip_connection, > this should use net/base/ip_endpoint.h instead of an undocumented string; ditto > for the other methods > > https://codereview.chromium.org/409883006/diff/1/components/gcm_driver/defaul... > File components/gcm_driver/default_gcm_app_handler.cc (right): > > https://codereview.chromium.org/409883006/diff/1/components/gcm_driver/defaul... > components/gcm_driver/default_gcm_app_handler.cc:56: LOG(ERROR) << "GCM > connected to " << ip_endpoint_string; > don't forget to change this back > > https://codereview.chromium.org/409883006/diff/1/components/gcm_driver/defaul... > components/gcm_driver/default_gcm_app_handler.cc:67: LOG(ERROR) << "GCM > disconnected"; > or this chromeos is not in components/DEPS because most components do not and should not depend on src/chromeos, however there is precedent for adding chromeos to a specific components/{MODULE}, e.g. components/usb_service/DEPS. That seems fine to add here. If this component will (or might) be used outside of Chrome OS, then I would do as Dan suggests or put the chromeos specific code in an _chromoes.cc implementation file.
https://codereview.chromium.org/409883006/diff/1/components/gcm_driver/defaul... File components/gcm_driver/default_gcm_app_handler.h (right): https://codereview.chromium.org/409883006/diff/1/components/gcm_driver/defaul... components/gcm_driver/default_gcm_app_handler.h:32: DISALLOW_COPY_AND_ASSIGN(DefaultGCMAppHandler); This should be last. https://codereview.chromium.org/409883006/diff/1/components/gcm_driver/defaul... components/gcm_driver/default_gcm_app_handler.h:35: const std::string& error); Do these need access to DefaultGCMAppHandler, or can the be file-local functions? (Generally file-local would be preferred, we can always pass extra data to these if wee need to using base::Bind)
https://codereview.chromium.org/409883006/diff/1/components/gcm_driver/defaul... File components/gcm_driver/default_gcm_app_handler.h (right): https://codereview.chromium.org/409883006/diff/1/components/gcm_driver/defaul... components/gcm_driver/default_gcm_app_handler.h:35: const std::string& error); On 2014/07/22 20:42:57, stevenjb wrote: > Do these need access to DefaultGCMAppHandler, or can the be file-local > functions? (Generally file-local would be preferred, we can always pass extra > data to these if wee need to using base::Bind) > I agree. Also please add documentation why they are there. I am not sure if you explained that someplace earlier or not. Why are you adding CrOS specific wake on lan code to DefaultAppHandler?
Filip, sorry if I didn't make it clearer, but this code is here merely to quickly indicate what I am trying to achieve. I have spent some time trying to structure this more sanely, but haven't found a way to. > a) define an interface under components/gcm_driver for registering wakeups with the OS Dan, by "defining an interface" do you mean adding public methods to an existing class, or creating a new class? I have considered the former, but I could not easily find a hook from chromeos code to any gcm object. Would it be acceptable to create a static global object that both the gcm driver and chromeos code can access? Then I could communicate through that (i.e. chromeos code would register gcm connection callbacks with that object, and the gcm driver, when instantiated, would pick up the callback from that object. > c) pass that early on into whatever creates GCM objects I can't find a way of doing this! GCM objects seem to be created dynamically in the context of Profiles, and I don't have a hook into that from chromeos code. Thanks! On 2014/07/23 20:18:03, fgorski wrote: > https://codereview.chromium.org/409883006/diff/1/components/gcm_driver/defaul... > File components/gcm_driver/default_gcm_app_handler.h (right): > > https://codereview.chromium.org/409883006/diff/1/components/gcm_driver/defaul... > components/gcm_driver/default_gcm_app_handler.h:35: const std::string& error); > On 2014/07/22 20:42:57, stevenjb wrote: > > Do these need access to DefaultGCMAppHandler, or can the be file-local > > functions? (Generally file-local would be preferred, we can always pass extra > > data to these if wee need to using base::Bind) > > > > I agree. Also please add documentation why they are there. > I am not sure if you explained that someplace earlier or not. Why are you adding > CrOS specific wake on lan code to DefaultAppHandler?
On 2014/07/23 23:23:13, Luigi Semenzato wrote: > Filip, sorry if I didn't make it clearer, but this code is here merely to > quickly indicate what I am trying to achieve. I have spent some time trying to > structure this more sanely, but haven't found a way to. > > > a) define an interface under components/gcm_driver for registering wakeups > with > the OS > > Dan, by "defining an interface" do you mean adding public methods to an existing > class, or creating a new class? I have considered the former, but I could not > easily find a hook from chromeos code to any gcm object. interface -> new abstract base class > Would it be acceptable to create a static global object that both the gcm driver > and chromeos code can access? Then I could communicate through that (i.e. > chromeos code would register gcm connection callbacks with that object, and the > gcm driver, when instantiated, would pick up the callback from that object. 1) define the interface in the gcm directory 2) define an implementation of the interface in c/b/chromeos 3) instantiate it as a member of ChromeBrowserMainPartsChromeos 4) add a method somewhere in gcm that you can use to register a given impl 5) make ChromeBrowserMainPartsChromeos pass its impl that method 6) make gcm call into the interface if an impl has been registered > > c) pass that early on into whatever creates GCM objects > > I can't find a way of doing this! GCM objects seem to be created dynamically in > the context of Profiles, and I don't have a hook into that from chromeos code. how are they created? where's the code? > Thanks! > > > On 2014/07/23 20:18:03, fgorski wrote: > > > https://codereview.chromium.org/409883006/diff/1/components/gcm_driver/defaul... > > File components/gcm_driver/default_gcm_app_handler.h (right): > > > > > https://codereview.chromium.org/409883006/diff/1/components/gcm_driver/defaul... > > components/gcm_driver/default_gcm_app_handler.h:35: const std::string& error); > > On 2014/07/22 20:42:57, stevenjb wrote: > > > Do these need access to DefaultGCMAppHandler, or can the be file-local > > > functions? (Generally file-local would be preferred, we can always pass > extra > > > data to these if wee need to using base::Bind) > > > > > > > I agree. Also please add documentation why they are there. > > I am not sure if you explained that someplace earlier or not. Why are you > adding > > CrOS specific wake on lan code to DefaultAppHandler?
Hi! Resuming this after vacation and other delays. On 2014/07/24 14:25:21, Daniel Erat wrote: > > Dan, by "defining an interface" do you mean adding public methods to an > existing > > class, or creating a new class? I have considered the former, but I could not > > easily find a hook from chromeos code to any gcm object. > > interface -> new abstract base class OK > 1) define the interface in the gcm directory > 2) define an implementation of the interface in c/b/chromeos > 3) instantiate it as a member of ChromeBrowserMainPartsChromeos > 4) add a method somewhere in gcm that you can use to register a given impl > 5) make ChromeBrowserMainPartsChromeos pass its impl that method > 6) make gcm call into the interface if an impl has been registered OK, but I have to physically store a callback into ChromeOS code somewhere that the GCM code can reach. GCM objects don't exist at init time, when I'd like to do the registration. So where do I store the callback? Is a static global object acceptable? > > > c) pass that early on into whatever creates GCM objects > > > > I can't find a way of doing this! GCM objects seem to be created dynamically > in > > the context of Profiles, and I don't have a hook into that from chromeos code. > > how are they created? where's the code? That's a difficult question. The chain of calls into OnConnected methods start from a GCMDriverDesktop instance. The GCMDriverDesktop object is created by BrowserProcessImpl::CreateGCMDriver() and saved locally in this->gcm_driver_. ./chrome/browser/browser_process_impl.cc:1019 CreateGCMDriver() is called: - in the constructor for a GCMProfileService; - by BrowserProcessImpl::gcm_driver(). GCMProfileService() is called: - in GCMPRofileServiceFactory::BuildServiceInstanceFor() BuildServiceInstanceFor has many instances (300+) so it is difficult to find the callers... but let's try. most likely called from RefcountedBrowserContextKeyedServiceFactory::GetServiceForBrowserContext() BrowserContextKeyedServiceFactory::GetServiceForBrowserContext() also called in PluginPrefsFactory::CreateForTestingProfile() (probably not interesting) gcm_driver() is called: nowhere. GetServiceForBrowserContext is called in 180 places. Most promising in GCMProfileServiceFactory::GetForProfile() There are 1823 instances of GetForProfile()... but after removing the spurious instances, I cannot find a good candidate for a call site. But in any case this can only be called after a profile has been created. > > > Thanks! > > > > > > On 2014/07/23 20:18:03, fgorski wrote: > > > > > > https://codereview.chromium.org/409883006/diff/1/components/gcm_driver/defaul... > > > File components/gcm_driver/default_gcm_app_handler.h (right): > > > > > > > > > https://codereview.chromium.org/409883006/diff/1/components/gcm_driver/defaul... > > > components/gcm_driver/default_gcm_app_handler.h:35: const std::string& > error); > > > On 2014/07/22 20:42:57, stevenjb wrote: > > > > Do these need access to DefaultGCMAppHandler, or can the be file-local > > > > functions? (Generally file-local would be preferred, we can always pass > > extra > > > > data to these if wee need to using base::Bind) > > > > > > > > > > I agree. Also please add documentation why they are there. > > > I am not sure if you explained that someplace earlier or not. Why are you > > adding > > > CrOS specific wake on lan code to DefaultAppHandler?
On Mon, Aug 18, 2014 at 2:09 PM, <semenzato@chromium.org> wrote: > Hi! Resuming this after vacation and other delays. > > On 2014/07/24 14:25:21, Daniel Erat wrote: > > > Dan, by "defining an interface" do you mean adding public methods to an >> existing >> > class, or creating a new class? I have considered the former, but I >> could >> > not > >> > easily find a hook from chromeos code to any gcm object. >> > > interface -> new abstract base class >> > > OK > > 1) define the interface in the gcm directory >> 2) define an implementation of the interface in c/b/chromeos >> 3) instantiate it as a member of ChromeBrowserMainPartsChromeos >> 4) add a method somewhere in gcm that you can use to register a given impl >> 5) make ChromeBrowserMainPartsChromeos pass its impl that method >> 6) make gcm call into the interface if an impl has been registered >> > > OK, but I have to physically store a callback into ChromeOS code somewhere > that > the GCM code can reach. GCM objects don't exist at init time, when I'd > like to > do the registration. So where do I store the callback? Is a static global > object acceptable? > It's up to the GCM owners, but yeah, exposing a function in GCM that Chrome can call to register a pointer to an object and storing that in e.g. a global in an anonymous namespace is what I'd try if there's no other clear place to register the Chrome object. > > > c) pass that early on into whatever creates GCM objects >> > >> > I can't find a way of doing this! GCM objects seem to be created >> > dynamically > >> in >> > the context of Profiles, and I don't have a hook into that from chromeos >> > code. > > how are they created? where's the code? >> > > That's a difficult question. The chain of calls into OnConnected methods > start > from a GCMDriverDesktop instance. > > The GCMDriverDesktop object is created by BrowserProcessImpl:: > CreateGCMDriver() > and saved locally in this->gcm_driver_. > ./chrome/browser/browser_process_impl.cc:1019 > > CreateGCMDriver() is called: > - in the constructor for a GCMProfileService; > - by BrowserProcessImpl::gcm_driver(). > > GCMProfileService() is called: > - in GCMPRofileServiceFactory::BuildServiceInstanceFor() > > BuildServiceInstanceFor has many instances (300+) so it is difficult to > find > the callers... but let's try. > > most likely called from > RefcountedBrowserContextKeyedServiceFactory::GetServiceForBrowserContext() > BrowserContextKeyedServiceFactory::GetServiceForBrowserContext() > > also called in > PluginPrefsFactory::CreateForTestingProfile() > (probably not interesting) > > gcm_driver() is called: nowhere. > > GetServiceForBrowserContext is called in 180 places. > > Most promising in GCMProfileServiceFactory::GetForProfile() > > There are 1823 instances of GetForProfile()... but after removing the > spurious > instances, I cannot find a good candidate for a call site. > > But in any case this can only be called after a profile has been created. > > > > Thanks! >> > >> > >> > On 2014/07/23 20:18:03, fgorski wrote: >> > > >> > >> > > https://codereview.chromium.org/409883006/diff/1/ > components/gcm_driver/default_gcm_app_handler.h > >> > > File components/gcm_driver/default_gcm_app_handler.h (right): >> > > >> > > >> > >> > > https://codereview.chromium.org/409883006/diff/1/ > components/gcm_driver/default_gcm_app_handler.h#newcode35 > >> > > components/gcm_driver/default_gcm_app_handler.h:35: const >> std::string& >> error); >> > > On 2014/07/22 20:42:57, stevenjb wrote: >> > > > Do these need access to DefaultGCMAppHandler, or can the be >> file-local >> > > > functions? (Generally file-local would be preferred, we can always >> pass >> > extra >> > > > data to these if wee need to using base::Bind) >> > > > >> > > >> > > I agree. Also please add documentation why they are there. >> > > I am not sure if you explained that someplace earlier or not. Why are >> you >> > adding >> > > CrOS specific wake on lan code to DefaultAppHandler? >> > > > > https://codereview.chromium.org/409883006/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/08/19 00:16:50, Daniel Erat wrote: > On Mon, Aug 18, 2014 at 2:09 PM, <mailto:semenzato@chromium.org> wrote: > > > Hi! Resuming this after vacation and other delays. > > > > On 2014/07/24 14:25:21, Daniel Erat wrote: > > > > > Dan, by "defining an interface" do you mean adding public methods to an > >> existing > >> > class, or creating a new class? I have considered the former, but I > >> could > >> > > not > > > >> > easily find a hook from chromeos code to any gcm object. > >> > > > > interface -> new abstract base class > >> > > > > OK > > > > 1) define the interface in the gcm directory > >> 2) define an implementation of the interface in c/b/chromeos > >> 3) instantiate it as a member of ChromeBrowserMainPartsChromeos > >> 4) add a method somewhere in gcm that you can use to register a given impl > >> 5) make ChromeBrowserMainPartsChromeos pass its impl that method > >> 6) make gcm call into the interface if an impl has been registered > >> > > > > OK, but I have to physically store a callback into ChromeOS code somewhere > > that > > the GCM code can reach. GCM objects don't exist at init time, when I'd > > like to > > do the registration. So where do I store the callback? Is a static global > > object acceptable? > > > > It's up to the GCM owners, but yeah, exposing a function in GCM that Chrome > can call to register a pointer to an object and storing that in e.g. a > global in an anonymous namespace is what I'd try if there's no other clear > place to register the Chrome object. > > > > > > c) pass that early on into whatever creates GCM objects > >> > > >> > I can't find a way of doing this! GCM objects seem to be created > >> > > dynamically > > > >> in > >> > the context of Profiles, and I don't have a hook into that from chromeos > >> > > code. > > > > how are they created? where's the code? > >> > > > > That's a difficult question. The chain of calls into OnConnected methods > > start > > from a GCMDriverDesktop instance. > > > > The GCMDriverDesktop object is created by BrowserProcessImpl:: > > CreateGCMDriver() > > and saved locally in this->gcm_driver_. > > ./chrome/browser/browser_process_impl.cc:1019 > > > > CreateGCMDriver() is called: > > - in the constructor for a GCMProfileService; > > - by BrowserProcessImpl::gcm_driver(). > > > > GCMProfileService() is called: > > - in GCMPRofileServiceFactory::BuildServiceInstanceFor() > > > > BuildServiceInstanceFor has many instances (300+) so it is difficult to > > find > > the callers... but let's try. > > > > most likely called from > > RefcountedBrowserContextKeyedServiceFactory::GetServiceForBrowserContext() > > BrowserContextKeyedServiceFactory::GetServiceForBrowserContext() > > > > also called in > > PluginPrefsFactory::CreateForTestingProfile() > > (probably not interesting) > > > > gcm_driver() is called: nowhere. > > > > GetServiceForBrowserContext is called in 180 places. > > > > Most promising in GCMProfileServiceFactory::GetForProfile() > > > > There are 1823 instances of GetForProfile()... but after removing the > > spurious > > instances, I cannot find a good candidate for a call site. > > > > But in any case this can only be called after a profile has been created. > > > > > > > Thanks! > >> > > >> > > >> > On 2014/07/23 20:18:03, fgorski wrote: > >> > > > >> > > >> > > > > https://codereview.chromium.org/409883006/diff/1/ > > components/gcm_driver/default_gcm_app_handler.h > > > >> > > File components/gcm_driver/default_gcm_app_handler.h (right): > >> > > > >> > > > >> > > >> > > > > https://codereview.chromium.org/409883006/diff/1/ > > components/gcm_driver/default_gcm_app_handler.h#newcode35 > > > >> > > components/gcm_driver/default_gcm_app_handler.h:35: const > >> std::string& > >> error); > >> > > On 2014/07/22 20:42:57, stevenjb wrote: > >> > > > Do these need access to DefaultGCMAppHandler, or can the be > >> file-local > >> > > > functions? (Generally file-local would be preferred, we can always > >> pass > >> > extra > >> > > > data to these if wee need to using base::Bind) > >> > > > > >> > > > >> > > I agree. Also please add documentation why they are there. > >> > > I am not sure if you explained that someplace earlier or not. Why are > >> you > >> > adding > >> > > CrOS specific wake on lan code to DefaultAppHandler? > >> > > > > > > > > https://codereview.chromium.org/409883006/ > > > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. 1. Could you propose an interface of that object, so that we know what exactly we are dealing with? 2. As for putting it into GCMDriver I think we could expose a relevant method, or pass it in construction or initialization. Once we know what we are dealing with it will be much easier to judge.
Thank you Filip. This, I think, is a cleaner way of organizing the code because the GCM driver doesn't have to know anything about DBus or shill. However, it uses static methods and variables. Please be aware that I am completely happy to rewrite everything. > 1. Could you propose an interface of that object, so that we know what exactly > we are dealing with? > 2. As for putting it into GCMDriver I think we could expose a relevant method, > or pass it in construction or initialization. Once we know what we are dealing > with it will be much easier to judge.
On 2014/08/19 23:25:03, Luigi Semenzato wrote:
> Thank you Filip. This, I think, is a cleaner way of organizing the code
because
> the GCM driver doesn't have to know anything about DBus or shill. However, it
> uses static methods and variables. Please be aware that I am completely happy
> to rewrite everything.
>
>
> > 1. Could you propose an interface of that object, so that we know what
exactly
> > we are dealing with?
> > 2. As for putting it into GCMDriver I think we could expose a relevant
method,
> > or pass it in construction or initialization. Once we know what we are
dealing
> > with it will be much easier to judge.
How about creating your own implementation of GCMAppHandler, in which you do
exactly what you need to be doing without the need for static hooks. Say it is
ChromeOSGCMAppHandler (implements only interesting methods related to
connection).
in ChromeBrowserMainChromeOS you can then add the app handler to the gcm_driver:
g_browser_process->gcm_driver()->AddAppHandler("ChromeOSGCMHandler",
your_handler_here);
(It will work even though you really have no app there, because the connection
events are broadcasted to every app handler)
Would there be a problem with that approach?
I am seeing your questions about creation of GCMDriver. (It is carried out by
GCMDesktopUtils, but can be triggered through browser process that you have
access to. Bartfab put it there to make it possible to do policy updates for
Chrome OS without having users signed in.)
https://codereview.chromium.org/301973009/
Do you think you can do it that way?
On 2014/08/20 23:23:51, fgorski wrote:
> On 2014/08/19 23:25:03, Luigi Semenzato wrote:
> > Thank you Filip. This, I think, is a cleaner way of organizing the code
> because
> > the GCM driver doesn't have to know anything about DBus or shill. However,
it
> > uses static methods and variables. Please be aware that I am completely
happy
> > to rewrite everything.
> >
> >
> > > 1. Could you propose an interface of that object, so that we know what
> exactly
> > > we are dealing with?
> > > 2. As for putting it into GCMDriver I think we could expose a relevant
> method,
> > > or pass it in construction or initialization. Once we know what we are
> dealing
> > > with it will be much easier to judge.
>
> How about creating your own implementation of GCMAppHandler, in which you do
> exactly what you need to be doing without the need for static hooks. Say it is
> ChromeOSGCMAppHandler (implements only interesting methods related to
> connection).
>
> in ChromeBrowserMainChromeOS you can then add the app handler to the
gcm_driver:
> g_browser_process->gcm_driver()->AddAppHandler("ChromeOSGCMHandler",
> your_handler_here);
> (It will work even though you really have no app there, because the connection
> events are broadcasted to every app handler)
>
> Would there be a problem with that approach?
>
> I am seeing your questions about creation of GCMDriver. (It is carried out by
> GCMDesktopUtils, but can be triggered through browser process that you have
> access to. Bartfab put it there to make it possible to do policy updates for
> Chrome OS without having users signed in.)
>
> https://codereview.chromium.org/301973009/
>
> Do you think you can do it that way?
Probably yes. However, this creates a GCM driver (and who knows what else,
indirectly) at init time, and it would be preferable to defer it, for boot
speed. Also, the GCM driver is created even if it's never used, which may be a
waste. Can you reassure me that neither concern is material?
Thanks!
Valid concern, Luigi. I think I am ok with having the statics in the default handler, I don't thing there is a large penalty for having those statics. We could also ifdef them to Chrome OS Nicolas, What do you think?
A few comments. https://codereview.chromium.org/409883006/diff/20001/components/gcm_driver/de... File components/gcm_driver/default_gcm_app_handler.h (right): https://codereview.chromium.org/409883006/diff/20001/components/gcm_driver/de... components/gcm_driver/default_gcm_app_handler.h:37: DISALLOW_COPY_AND_ASSIGN(DefaultGCMAppHandler); following stevenjb, move this to the bottom. https://codereview.chromium.org/409883006/diff/20001/components/gcm_driver/de... components/gcm_driver/default_gcm_app_handler.h:38: static void NullCallback(const std::string& result); I think you don't need NullCallback or ErrorCallback anymore. https://codereview.chromium.org/409883006/diff/20001/components/gcm_driver/de... components/gcm_driver/default_gcm_app_handler.h:42: static void (*on_connected_cb_)(const net::IPEndPoint& ip_endpoint); could you typedef and document both of the signatures?
Can't you register the callbacks from within the GCMProfileService (possibly by adding a getter for the default app handler)? That way you can use normal base::Callbacks and don't have to deal with static methods. I'm fine with having code that depends on ChromeOS in the GCMProfileService. As long as we don't put it in any component code.
On 2014/08/21 19:03:09, Nicolas Zea wrote: > Can't you register the callbacks from within the GCMProfileService (possibly by > adding a getter for the default app handler)? That way you can use normal > base::Callbacks and don't have to deal with static methods. > > I'm fine with having code that depends on ChromeOS in the GCMProfileService. As > long as we don't put it in any component code. Let me try your suggestion next. In the meanwhile I have uploaded a change for Filip's suggestion, just for reference. I don't think it's worth reviewing it though, unless we revisit that choice. It's probably still too soon for detailed comments, thanks.
On 2014/08/21 19:59:04, Luigi Semenzato wrote: > On 2014/08/21 19:03:09, Nicolas Zea wrote: > > Can't you register the callbacks from within the GCMProfileService (possibly > by > > adding a getter for the default app handler)? That way you can use normal > > base::Callbacks and don't have to deal with static methods. > > > > I'm fine with having code that depends on ChromeOS in the GCMProfileService. > As > > long as we don't put it in any component code. > > Let me try your suggestion next. In the meanwhile I have uploaded a change for > Filip's suggestion, just for reference. I don't think it's worth reviewing it > though, unless we revisit that choice. > > It's probably still too soon for detailed comments, thanks. So: I combined Filip's and Nicolas's suggestions by moving the chromeos code to the GCMProfileService constructor, but also adding a ChromeOS-specific app handler. This code compiles but I haven't tried to run it yet. If everybody agrees on the general idea, I will test it, then apply some of the earlier suggestions for changes, then request a detailed review. Also, it will need some further modification---the OnDisconnect callback also needs to be passed the ip_endpoint, or else we won't (easily) know which rule to remove from the NIC. Thanks! Thanks!
https://codereview.chromium.org/409883006/diff/60001/chrome/browser/services/... File chrome/browser/services/gcm/gcm_profile_service.cc (right): https://codereview.chromium.org/409883006/diff/60001/chrome/browser/services/... chrome/browser/services/gcm/gcm_profile_service.cc:129: new gcm::ChromeOSGCMAppHandler()); Note that this will leak, as the driver doesn't take ownership of any app handlers provided to it. Also, we should be careful about which app id we give it, and make sure it uses a namespace reserved for internal apps. Filip, I believe Android already does something similar, by reserving certain prefixes?
IPEndpoint in Disconnect: As for needing to have ip_endpoint in disconnect, that could easily be kept in the ChromeOSGCMAppHandler, so the tracking of state would be happening there. Leaking app handler: CrOS app handler can probably be a field inside of the profile service on Chrome OS, to avoid the memory leak. Question: Is there a case where more than a single profile would be running in Chrome OS? In that case we'll have multiple connections, and perhaps we want to handle that case in some special way. https://codereview.chromium.org/409883006/diff/60001/chrome/browser/services/... File chrome/browser/services/gcm/gcm_profile_service.cc (right): https://codereview.chromium.org/409883006/diff/60001/chrome/browser/services/... chrome/browser/services/gcm/gcm_profile_service.cc:129: new gcm::ChromeOSGCMAppHandler()); On 2014/08/21 21:22:19, Nicolas Zea wrote: > Note that this will leak, as the driver doesn't take ownership of any app > handlers provided to it. Also, we should be careful about which app id we give > it, and make sure it uses a namespace reserved for internal apps. > > Filip, I believe Android already does something similar, by reserving certain > prefixes? there is no namespace specified for that purpose. Sync uses com.google.chrome.invalidations for invalidations we can probably have one for chrome OS like that. com.google.chrome.chromeos (or something specific to sleep)
On 2014/08/21 21:38:26, fgorski wrote: > IPEndpoint in Disconnect: > As for needing to have ip_endpoint in disconnect, that could easily be kept in > the ChromeOSGCMAppHandler, so the tracking of state would be happening there. Sure, as long as one GCMProfileService is created for each profile. > Leaking app handler: > CrOS app handler can probably be a field inside of the profile service on Chrome > OS, to avoid the memory leak. Sure. And call its destructor in GCMProfileService::ShutDown(). > Question: > Is there a case where more than a single profile would be running in Chrome OS? > In that case we'll have multiple connections, and perhaps we want to handle that > case in some special way. Yes, Chrome OS supports multiple profiles. But as long as a profile service is created for each profile, we are OK. Is that the case? > > https://codereview.chromium.org/409883006/diff/60001/chrome/browser/services/... > File chrome/browser/services/gcm/gcm_profile_service.cc (right): > > https://codereview.chromium.org/409883006/diff/60001/chrome/browser/services/... > chrome/browser/services/gcm/gcm_profile_service.cc:129: new > gcm::ChromeOSGCMAppHandler()); > On 2014/08/21 21:22:19, Nicolas Zea wrote: > > Note that this will leak, as the driver doesn't take ownership of any app > > handlers provided to it. Also, we should be careful about which app id we give > > it, and make sure it uses a namespace reserved for internal apps. > > > > Filip, I believe Android already does something similar, by reserving certain > > prefixes? > > there is no namespace specified for that purpose. > Sync uses com.google.chrome.invalidations for invalidations > we can probably have one for chrome OS like that. > com.google.chrome.chromeos (or something specific to sleep) With multiple profiles, do the app ids have to be unique? Thanks!
On 2014/08/21 21:52:25, Luigi Semenzato wrote: > On 2014/08/21 21:38:26, fgorski wrote: > > IPEndpoint in Disconnect: > > As for needing to have ip_endpoint in disconnect, that could easily be kept in > > the ChromeOSGCMAppHandler, so the tracking of state would be happening there. > > Sure, as long as one GCMProfileService is created for each profile. Each profile has its own GCMProfileService and each one has its own connection. > > > Leaking app handler: > > CrOS app handler can probably be a field inside of the profile service on > Chrome > > OS, to avoid the memory leak. > > Sure. And call its destructor in GCMProfileService::ShutDown(). Yes, also probably remove app handler. You can alternatively depend on destructor of GCMProfileService, but consider the timing of when things should happen. Shutdown feels right. > > > Question: > > Is there a case where more than a single profile would be running in Chrome > OS? > > In that case we'll have multiple connections, and perhaps we want to handle > that > > case in some special way. > > Yes, Chrome OS supports multiple profiles. But as long as a profile service is > created for each profile, we are OK. Is that the case? To confirm that is what it looks like (I have google and gmail profiles signed in and switch between them. Both of them have a distinct device ID on chrome::gcm-insternals page) > > > > > > https://codereview.chromium.org/409883006/diff/60001/chrome/browser/services/... > > File chrome/browser/services/gcm/gcm_profile_service.cc (right): > > > > > https://codereview.chromium.org/409883006/diff/60001/chrome/browser/services/... > > chrome/browser/services/gcm/gcm_profile_service.cc:129: new > > gcm::ChromeOSGCMAppHandler()); > > On 2014/08/21 21:22:19, Nicolas Zea wrote: > > > Note that this will leak, as the driver doesn't take ownership of any app > > > handlers provided to it. Also, we should be careful about which app id we > give > > > it, and make sure it uses a namespace reserved for internal apps. > > > > > > Filip, I believe Android already does something similar, by reserving > certain > > > prefixes? > > > > there is no namespace specified for that purpose. > > Sync uses com.google.chrome.invalidations for invalidations > > we can probably have one for chrome OS like that. > > com.google.chrome.chromeos (or something specific to sleep) > > With multiple profiles, do the app ids have to be unique? No you can have the same app in multiple profiles. E.g. Sync, hangouts. They are scoped to Android Device (which every profile in chrome is) > > Thanks!
https://codereview.chromium.org/409883006/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right): https://codereview.chromium.org/409883006/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:777: void ChromeBrowserMainPartsChromeos::OnGCMDisconnected() { nit: add a "static" comment here too https://codereview.chromium.org/409883006/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/chrome_browser_main_chromeos.h (right): https://codereview.chromium.org/409883006/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/chrome_browser_main_chromeos.h:13: #include "net/base/ip_endpoint.h" nit: you don't need to include this in the header; forward-declare net::IPEndPoint instead https://codereview.chromium.org/409883006/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/chrome_browser_main_chromeos.h:65: static void GCMErrorCallback(const std::string &error_name, nit: '&' should go to the left of the space on this line and the next https://codereview.chromium.org/409883006/diff/60001/chrome/browser/services/... File chrome/browser/services/gcm/chromeos_gcm_app_handler.cc (right): https://codereview.chromium.org/409883006/diff/60001/chrome/browser/services/... chrome/browser/services/gcm/chromeos_gcm_app_handler.cc:30: GetShillManagerClient()-> nit: indent this four spaces beyond the previous line https://codereview.chromium.org/409883006/diff/60001/chrome/browser/services/... chrome/browser/services/gcm/chromeos_gcm_app_handler.cc:32: ip_endpoint_string, nit: indent four spaces, not two https://codereview.chromium.org/409883006/diff/60001/chrome/browser/services/... File chrome/browser/services/gcm/chromeos_gcm_app_handler.h (right): https://codereview.chromium.org/409883006/diff/60001/chrome/browser/services/... chrome/browser/services/gcm/chromeos_gcm_app_handler.h:10: #include "net/base/ip_endpoint.h" nit: forward-declare this too https://codereview.chromium.org/409883006/diff/60001/chromeos/dbus/shill_mana... File chromeos/dbus/shill_manager_client.h (right): https://codereview.chromium.org/409883006/diff/60001/chromeos/dbus/shill_mana... chromeos/dbus/shill_manager_client.h:227: const std::string& ip_connection, think i mentioned this earlier, but please either use a more-specific type than std::string for this or document exactly what the format is or where's it's defined https://codereview.chromium.org/409883006/diff/60001/components/gcm_driver/de... File components/gcm_driver/default_gcm_app_handler.h (right): https://codereview.chromium.org/409883006/diff/60001/components/gcm_driver/de... components/gcm_driver/default_gcm_app_handler.h:10: #include "net/base/ip_endpoint.h" nit: forward-declare https://codereview.chromium.org/409883006/diff/60001/components/gcm_driver/de... components/gcm_driver/default_gcm_app_handler.h:34: static void NullCallback(const std::string& result); delete these? i don't see them defined in the .cc file. private static methods should usually be functions in an anonymous namespace in the .cc file instead.
I think we can start worrying about details now. (I fixed some already.) I am not too happy about the #ifdef OS_CHROMEOS but I can live with them. Thanks! https://codereview.chromium.org/409883006/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right): https://codereview.chromium.org/409883006/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:777: void ChromeBrowserMainPartsChromeos::OnGCMDisconnected() { On 2014/08/21 22:17:37, Daniel Erat wrote: > nit: add a "static" comment here too Sorry---this is going away. https://codereview.chromium.org/409883006/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/chrome_browser_main_chromeos.h (right): https://codereview.chromium.org/409883006/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/chrome_browser_main_chromeos.h:65: static void GCMErrorCallback(const std::string &error_name, On 2014/08/21 22:17:37, Daniel Erat wrote: > nit: '&' should go to the left of the space on this line and the next Done. https://codereview.chromium.org/409883006/diff/60001/chrome/browser/services/... File chrome/browser/services/gcm/chromeos_gcm_app_handler.cc (right): https://codereview.chromium.org/409883006/diff/60001/chrome/browser/services/... chrome/browser/services/gcm/chromeos_gcm_app_handler.cc:30: GetShillManagerClient()-> On 2014/08/21 22:17:37, Daniel Erat wrote: > nit: indent this four spaces beyond the previous line Done. https://codereview.chromium.org/409883006/diff/60001/chrome/browser/services/... chrome/browser/services/gcm/chromeos_gcm_app_handler.cc:32: ip_endpoint_string, On 2014/08/21 22:17:37, Daniel Erat wrote: > nit: indent four spaces, not two Done. https://codereview.chromium.org/409883006/diff/60001/chrome/browser/services/... File chrome/browser/services/gcm/chromeos_gcm_app_handler.h (right): https://codereview.chromium.org/409883006/diff/60001/chrome/browser/services/... chrome/browser/services/gcm/chromeos_gcm_app_handler.h:10: #include "net/base/ip_endpoint.h" On 2014/08/21 22:17:37, Daniel Erat wrote: > nit: forward-declare this too Done. https://codereview.chromium.org/409883006/diff/60001/chrome/browser/services/... File chrome/browser/services/gcm/gcm_profile_service.cc (right): https://codereview.chromium.org/409883006/diff/60001/chrome/browser/services/... chrome/browser/services/gcm/gcm_profile_service.cc:129: new gcm::ChromeOSGCMAppHandler()); On 2014/08/21 21:38:26, fgorski wrote: > On 2014/08/21 21:22:19, Nicolas Zea wrote: > > Note that this will leak, as the driver doesn't take ownership of any app > > handlers provided to it. Also, we should be careful about which app id we give > > it, and make sure it uses a namespace reserved for internal apps. > > > > Filip, I believe Android already does something similar, by reserving certain > > prefixes? > > there is no namespace specified for that purpose. > Sync uses com.google.chrome.invalidations for invalidations > we can probably have one for chrome OS like that. > com.google.chrome.chromeos (or something specific to sleep) Done, although I suspect the app id needs to be defined as a constant. https://codereview.chromium.org/409883006/diff/60001/chromeos/dbus/shill_mana... File chromeos/dbus/shill_manager_client.h (right): https://codereview.chromium.org/409883006/diff/60001/chromeos/dbus/shill_mana... chromeos/dbus/shill_manager_client.h:227: const std::string& ip_connection, On 2014/08/21 22:17:38, Daniel Erat wrote: > think i mentioned this earlier, but please either use a more-specific type than > std::string for this or document exactly what the format is or where's it's > defined Sorry, I was still hoping to have coarse-level reviews, so I haven't changed these before, but I am doing it now. Done. https://codereview.chromium.org/409883006/diff/60001/components/gcm_driver/de... File components/gcm_driver/default_gcm_app_handler.h (right): https://codereview.chromium.org/409883006/diff/60001/components/gcm_driver/de... components/gcm_driver/default_gcm_app_handler.h:10: #include "net/base/ip_endpoint.h" On 2014/08/21 22:17:38, Daniel Erat wrote: > nit: forward-declare Done. https://codereview.chromium.org/409883006/diff/60001/components/gcm_driver/de... components/gcm_driver/default_gcm_app_handler.h:34: static void NullCallback(const std::string& result); On 2014/08/21 22:17:38, Daniel Erat wrote: > delete these? i don't see them defined in the .cc file. > > private static methods should usually be functions in an anonymous namespace in > the .cc file instead. Done.
lgtm for chromeos/ after some comments are addressed; i'll let the people who work on gcm review the rest of the code https://codereview.chromium.org/409883006/diff/80001/chromeos/dbus/fake_shill... File chromeos/dbus/fake_shill_manager_client.h (right): https://codereview.chromium.org/409883006/diff/80001/chromeos/dbus/fake_shill... chromeos/dbus/fake_shill_manager_client.h:16: nit: get rid of blank lines within this namespace: namespace net { class IPEndPoint; } https://codereview.chromium.org/409883006/diff/80001/chromeos/dbus/mock_shill... File chromeos/dbus/mock_shill_manager_client.h (right): https://codereview.chromium.org/409883006/diff/80001/chromeos/dbus/mock_shill... chromeos/dbus/mock_shill_manager_client.h:70: void(const std::string& ip_connection, please double-check that chromeos_unittests compiles -- these need to be updated https://codereview.chromium.org/409883006/diff/80001/chromeos/dbus/shill_mana... File chromeos/dbus/shill_manager_client.cc (right): https://codereview.chromium.org/409883006/diff/80001/chromeos/dbus/shill_mana... chromeos/dbus/shill_manager_client.cc:246: dbus::MessageWriter writer(&method_call); you don't need this MessageWriter https://codereview.chromium.org/409883006/diff/80001/chromeos/dbus/shill_mana... File chromeos/dbus/shill_manager_client.h (right): https://codereview.chromium.org/409883006/diff/80001/chromeos/dbus/shill_mana... chromeos/dbus/shill_manager_client.h:23: nit: delete blank lines within namespace
Thank you. chromeos_unittests now compiles. https://codereview.chromium.org/409883006/diff/80001/chromeos/dbus/fake_shill... File chromeos/dbus/fake_shill_manager_client.h (right): https://codereview.chromium.org/409883006/diff/80001/chromeos/dbus/fake_shill... chromeos/dbus/fake_shill_manager_client.h:16: On 2014/08/23 14:44:52, Daniel Erat wrote: > nit: get rid of blank lines within this namespace: > > namespace net { > class IPEndPoint; > } Done. https://codereview.chromium.org/409883006/diff/80001/chromeos/dbus/mock_shill... File chromeos/dbus/mock_shill_manager_client.h (right): https://codereview.chromium.org/409883006/diff/80001/chromeos/dbus/mock_shill... chromeos/dbus/mock_shill_manager_client.h:70: void(const std::string& ip_connection, On 2014/08/23 14:44:52, Daniel Erat wrote: > please double-check that chromeos_unittests compiles -- these need to be updated Done. https://codereview.chromium.org/409883006/diff/80001/chromeos/dbus/shill_mana... File chromeos/dbus/shill_manager_client.cc (right): https://codereview.chromium.org/409883006/diff/80001/chromeos/dbus/shill_mana... chromeos/dbus/shill_manager_client.cc:246: dbus::MessageWriter writer(&method_call); On 2014/08/23 14:44:52, Daniel Erat wrote: > you don't need this MessageWriter Thanks. (Why no compiler warning? Possible side effects in constructor? Still...) https://codereview.chromium.org/409883006/diff/80001/chromeos/dbus/shill_mana... File chromeos/dbus/shill_manager_client.h (right): https://codereview.chromium.org/409883006/diff/80001/chromeos/dbus/shill_mana... chromeos/dbus/shill_manager_client.h:23: On 2014/08/23 14:44:52, Daniel Erat wrote: > nit: delete blank lines within namespace Done.
Hi, Luigi, It looks like the patch is almost there. We've put some more thought into it and it seems that there is a way to make sure you don't trigger a GCM connection by adding Chrome OS App Handler. I've filed http://crbug.com/407841 for that. We'll split GCMConnectionObserver from GCMAppHandler and update your code once that happens. That will prevent the situation where you would start GCM Connection. Other than that I think you are good as soon as you address the comments. Sorry for taking so long. /Filip https://codereview.chromium.org/409883006/diff/100001/chrome/browser/services... File chrome/browser/services/gcm/gcm_profile_service.cc (right): https://codereview.chromium.org/409883006/diff/100001/chrome/browser/services... chrome/browser/services/gcm/gcm_profile_service.cc:33: const char kGCMChromeOSAppHandlerId = "com.google.chrome.chromeos"; https://codereview.chromium.org/409883006/diff/100001/chrome/browser/services... chrome/browser/services/gcm/gcm_profile_service.cc:129: driver_->AddAppHandler("com.google.chrome.chromeos", chromeos_app_handler_); chromeos_app_handler_.reset(new gcm::ChromeOSGCMAppHandler()); driver_->AddAppHandler(kGCMChromeOSAppHandlerId, chromeos_app_handler_.get()); https://codereview.chromium.org/409883006/diff/100001/chrome/browser/services... chrome/browser/services/gcm/gcm_profile_service.cc:172: delete chromeos_app_handler_; driver_->RemoveAppHandler(kGCMChromeOSAppHandlerId); chromeos_app_handler_.reset(); https://codereview.chromium.org/409883006/diff/100001/chrome/browser/services... File chrome/browser/services/gcm/gcm_profile_service.h (right): https://codereview.chromium.org/409883006/diff/100001/chrome/browser/services... chrome/browser/services/gcm/gcm_profile_service.h:90: GCMAppHandler* chromeos_app_handler_; use a scoped_ptr. https://codereview.chromium.org/409883006/diff/100001/components/gcm_driver/d... File components/gcm_driver/default_gcm_app_handler.cc (right): https://codereview.chromium.org/409883006/diff/100001/components/gcm_driver/d... components/gcm_driver/default_gcm_app_handler.cc:7: #include "base/callback.h" not needed. https://codereview.chromium.org/409883006/diff/100001/components/gcm_driver/d... File components/gcm_driver/default_gcm_app_handler.h (right): https://codereview.chromium.org/409883006/diff/100001/components/gcm_driver/d... components/gcm_driver/default_gcm_app_handler.h:11: namespace net { I think this is already forward declared in gcm_client and included through gcm_app_handler
On 2014/08/26 22:26:08, fgorski wrote: > Hi, Luigi, > > It looks like the patch is almost there. > > We've put some more thought into it and it seems that there is a way to make > sure you don't trigger a GCM connection by adding Chrome OS App Handler. > I've filed http://crbug.com/407841 for that. > > We'll split GCMConnectionObserver from GCMAppHandler and update your code once > that happens. That will prevent the situation where you would start GCM > Connection. Other than that I think you are good as soon as you address the > comments. > > Sorry for taking so long. > > /Filip > > https://codereview.chromium.org/409883006/diff/100001/chrome/browser/services... > File chrome/browser/services/gcm/gcm_profile_service.cc (right): > > https://codereview.chromium.org/409883006/diff/100001/chrome/browser/services... > chrome/browser/services/gcm/gcm_profile_service.cc:33: > const char kGCMChromeOSAppHandlerId = "com.google.chrome.chromeos"; > > https://codereview.chromium.org/409883006/diff/100001/chrome/browser/services... > chrome/browser/services/gcm/gcm_profile_service.cc:129: > driver_->AddAppHandler("com.google.chrome.chromeos", chromeos_app_handler_); > chromeos_app_handler_.reset(new gcm::ChromeOSGCMAppHandler()); > driver_->AddAppHandler(kGCMChromeOSAppHandlerId, chromeos_app_handler_.get()); > > https://codereview.chromium.org/409883006/diff/100001/chrome/browser/services... > chrome/browser/services/gcm/gcm_profile_service.cc:172: delete > chromeos_app_handler_; > driver_->RemoveAppHandler(kGCMChromeOSAppHandlerId); > chromeos_app_handler_.reset(); > > https://codereview.chromium.org/409883006/diff/100001/chrome/browser/services... > File chrome/browser/services/gcm/gcm_profile_service.h (right): > > https://codereview.chromium.org/409883006/diff/100001/chrome/browser/services... > chrome/browser/services/gcm/gcm_profile_service.h:90: GCMAppHandler* > chromeos_app_handler_; > use a scoped_ptr. > > https://codereview.chromium.org/409883006/diff/100001/components/gcm_driver/d... > File components/gcm_driver/default_gcm_app_handler.cc (right): > > https://codereview.chromium.org/409883006/diff/100001/components/gcm_driver/d... > components/gcm_driver/default_gcm_app_handler.cc:7: #include "base/callback.h" > not needed. > > https://codereview.chromium.org/409883006/diff/100001/components/gcm_driver/d... > File components/gcm_driver/default_gcm_app_handler.h (right): > > https://codereview.chromium.org/409883006/diff/100001/components/gcm_driver/d... > components/gcm_driver/default_gcm_app_handler.h:11: namespace net { > I think this is already forward declared in gcm_client and included through > gcm_app_handler GCMConnectionObserver in review here: https://codereview.chromium.org/515763002/
Thanks. I made the changes, and also made a slight change to the D-Bus protocol with shill because the networking folks have decided to make the same change on their side. https://codereview.chromium.org/409883006/diff/100001/chrome/browser/services... File chrome/browser/services/gcm/gcm_profile_service.cc (right): https://codereview.chromium.org/409883006/diff/100001/chrome/browser/services... chrome/browser/services/gcm/gcm_profile_service.cc:33: On 2014/08/26 22:26:08, fgorski wrote: > const char kGCMChromeOSAppHandlerId = "com.google.chrome.chromeos"; Done. https://codereview.chromium.org/409883006/diff/100001/chrome/browser/services... chrome/browser/services/gcm/gcm_profile_service.cc:129: driver_->AddAppHandler("com.google.chrome.chromeos", chromeos_app_handler_); On 2014/08/26 22:26:08, fgorski wrote: > chromeos_app_handler_.reset(new gcm::ChromeOSGCMAppHandler()); > driver_->AddAppHandler(kGCMChromeOSAppHandlerId, chromeos_app_handler_.get()); Done. https://codereview.chromium.org/409883006/diff/100001/chrome/browser/services... chrome/browser/services/gcm/gcm_profile_service.cc:172: delete chromeos_app_handler_; On 2014/08/26 22:26:08, fgorski wrote: > driver_->RemoveAppHandler(kGCMChromeOSAppHandlerId); > chromeos_app_handler_.reset(); Done. https://codereview.chromium.org/409883006/diff/100001/chrome/browser/services... File chrome/browser/services/gcm/gcm_profile_service.h (right): https://codereview.chromium.org/409883006/diff/100001/chrome/browser/services... chrome/browser/services/gcm/gcm_profile_service.h:90: GCMAppHandler* chromeos_app_handler_; On 2014/08/26 22:26:08, fgorski wrote: > use a scoped_ptr. Done. https://codereview.chromium.org/409883006/diff/100001/components/gcm_driver/d... File components/gcm_driver/default_gcm_app_handler.cc (right): https://codereview.chromium.org/409883006/diff/100001/components/gcm_driver/d... components/gcm_driver/default_gcm_app_handler.cc:7: #include "base/callback.h" On 2014/08/26 22:26:08, fgorski wrote: > not needed. Done. https://codereview.chromium.org/409883006/diff/100001/components/gcm_driver/d... File components/gcm_driver/default_gcm_app_handler.h (right): https://codereview.chromium.org/409883006/diff/100001/components/gcm_driver/d... components/gcm_driver/default_gcm_app_handler.h:11: namespace net { On 2014/08/26 22:26:08, fgorski wrote: > I think this is already forward declared in gcm_client and included through > gcm_app_handler Done.
chromeos_unittests still compiles.
Patch doesn't apply because of the GCMAppHandler split into handler and connection observer https://code.google.com/p/chromium/codesearch#chromium/src/components/gcm_dri... And that is what you should be extending. This will make your GCM changes very contained though. https://codereview.chromium.org/409883006/diff/120001/chrome/browser/services... File chrome/browser/services/gcm/chromeos_gcm_app_handler.h (right): https://codereview.chromium.org/409883006/diff/120001/chrome/browser/services... chrome/browser/services/gcm/chromeos_gcm_app_handler.h:17: class ChromeOSGCMAppHandler : public GCMAppHandler { should be: class ChromeOSGCMConnectionObserver : public GCMConnectionObserver This will make that part much smaller and allow it to apply. You no longer need to inherit from GCMAppHandler.
src/chromeos lgtm
Thank you all. I changed the code from app handler to connection observer and tested it. PTAL!
LGTM with nits. Thanks for your patience, Luigi. https://codereview.chromium.org/409883006/diff/160001/chrome/browser/services... File chrome/browser/services/gcm/chromeos_gcm_connection_observer.h (right): https://codereview.chromium.org/409883006/diff/160001/chrome/browser/services... chrome/browser/services/gcm/chromeos_gcm_connection_observer.h:10: #include "net/base/ip_endpoint.h" nit: you can forward declare it here I think. https://codereview.chromium.org/409883006/diff/160001/chrome/browser/services... chrome/browser/services/gcm/chromeos_gcm_connection_observer.h:11: nit: extra line not necessary https://codereview.chromium.org/409883006/diff/160001/chrome/browser/services... chrome/browser/services/gcm/chromeos_gcm_connection_observer.h:19: virtual void OnConnected(const net::IPEndPoint& ip_endpoint) OVERRIDE; nit: add comment above: // gcm::GCMConnectionObserver implementation:
Thank you very much Filip! https://codereview.chromium.org/409883006/diff/160001/chrome/browser/services... File chrome/browser/services/gcm/chromeos_gcm_connection_observer.h (right): https://codereview.chromium.org/409883006/diff/160001/chrome/browser/services... chrome/browser/services/gcm/chromeos_gcm_connection_observer.h:10: #include "net/base/ip_endpoint.h" On 2014/09/08 21:48:18, fgorski wrote: > nit: you can forward declare it here I think. I don't think so, I have a class member of type IPEndPoint below. https://codereview.chromium.org/409883006/diff/160001/chrome/browser/services... chrome/browser/services/gcm/chromeos_gcm_connection_observer.h:11: On 2014/09/08 21:48:18, fgorski wrote: > nit: extra line not necessary Done. https://codereview.chromium.org/409883006/diff/160001/chrome/browser/services... chrome/browser/services/gcm/chromeos_gcm_connection_observer.h:19: virtual void OnConnected(const net::IPEndPoint& ip_endpoint) OVERRIDE; On 2014/09/08 21:48:18, fgorski wrote: > nit: add comment above: > > // gcm::GCMConnectionObserver implementation: Done.
The CQ bit was checked by semenzato@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/409883006/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2014/09/11 20:41:22, I haz the power (commit-bot) wrote: > Try jobs failed on following builders: > linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux > (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) This seems related to the code, but I don't understand how my changes could make these tests fail. New failures: GCMProfileServiceTest.CreateGCMProfileServiceBeforeSignIn GCMProfileServiceTest.RegisterAndUnregister GCMProfileServiceTest.Send ExtensionGCMAppHandlerTest.UnregisterOnExtensionUninstall GCMProfileServiceTest.SignInAgain @@@STEP_CURSOR unit_tests@@@ @@@STEP_TEXT@<br/>failures:<br/>GCMProfileServiceTest.CreateGCMProfileServiceBeforeSignIn<br/>GCMProfileServiceTest.RegisterAndUnregister<br/>GCMProfileServiceTest.Send<br/>ExtensionGCMAppHandlerTest.UnregisterOnExtensionUninstall<br/>GCMProfileServiceTest.SignInAgain<br/>@@@ @@@STEP_LOG_LINE@python.inline@@@@
On 2014/09/11 21:00:32, Luigi Semenzato wrote: > On 2014/09/11 20:41:22, I haz the power (commit-bot) wrote: > > Try jobs failed on following builders: > > linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux > > > (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) > > This seems related to the code, but I don't understand how my changes could make > these tests fail. > > > New failures: > GCMProfileServiceTest.CreateGCMProfileServiceBeforeSignIn > GCMProfileServiceTest.RegisterAndUnregister > GCMProfileServiceTest.Send > ExtensionGCMAppHandlerTest.UnregisterOnExtensionUninstall > GCMProfileServiceTest.SignInAgain > > @@@STEP_CURSOR unit_tests@@@ > > @@@STEP_TEXT@<br/>failures:<br/>GCMProfileServiceTest.CreateGCMProfileServiceBeforeSignIn<br/>GCMProfileServiceTest.RegisterAndUnregister<br/>GCMProfileServiceTest.Send<br/>ExtensionGCMAppHandlerTest.UnregisterOnExtensionUninstall<br/>GCMProfileServiceTest.SignInAgain<br/>@@@ > > mailto:@@@STEP_LOG_LINE@python.inline@@@@ All of them hit a dbus thread manager DCHECK [28906:28906:0911/131601:526028966632:FATAL:dbus_thread_manager.cc(418)] Check failed: g_dbus_thread_manager. DBusThreadManager::Get() called before Initialize()
> All of them hit a dbus thread manager DCHECK > [28906:28906:0911/131601:526028966632:FATAL:dbus_thread_manager.cc(418)] Check > failed: g_dbus_thread_manager. DBusThreadManager::Get() called before > Initialize() Thank you Filip. That makes sense. I'll fix it and run the unit tests.
The CQ bit was checked by semenzato@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/409883006/200001
https://codereview.chromium.org/409883006/diff/200001/chrome/browser/services... File chrome/browser/services/gcm/gcm_profile_service_unittest.cc (right): https://codereview.chromium.org/409883006/diff/200001/chrome/browser/services... chrome/browser/services/gcm/gcm_profile_service_unittest.cc:126: (void) chromeos::DBusThreadManager::GetSetterForTesting(); This is causing the build to fail on non-chrome os systems: obj/chrome/browser/services/gcm/unit_tests.gcm_profile_service_unittest.o:../../chrome/browser/services/gcm/gcm_profile_service_unittest.cc:function gcm::GCMProfileServiceTest::SetUp():error: undefined reference to 'chromeos::DBusThreadManager::GetSetterForTesting()' obj/chrome/browser/services/gcm/unit_tests.gcm_profile_service_unittest.o:../../chrome/browser/services/gcm/gcm_profile_service_unittest.cc:function gcm::GCMProfileServiceTest::SetUp():error: undefined reference to 'chromeos::DBusThreadManagerSetter::~DBusThreadManagerSetter()' You could use an #if defined(OS_CHROMEOS) here or you could move the ChromeOSGCMConnectionObserver into chrome/browser/chromeos/net and have it be owned by ChromeBrowserMainPartsChromeos. My preference is for the latter since then you probably wouldn't need to init the DBusThreadManager here.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/409883006/diff/200001/chrome/browser/services... File chrome/browser/services/gcm/gcm_profile_service_unittest.cc (right): https://codereview.chromium.org/409883006/diff/200001/chrome/browser/services... chrome/browser/services/gcm/gcm_profile_service_unittest.cc:126: (void) chromeos::DBusThreadManager::GetSetterForTesting(); On 2014/09/12 00:52:16, chirantan wrote: > This is causing the build to fail on non-chrome os systems: > > obj/chrome/browser/services/gcm/unit_tests.gcm_profile_service_unittest.o:../../chrome/browser/services/gcm/gcm_profile_service_unittest.cc:function > gcm::GCMProfileServiceTest::SetUp():error: undefined reference to > 'chromeos::DBusThreadManager::GetSetterForTesting()' > obj/chrome/browser/services/gcm/unit_tests.gcm_profile_service_unittest.o:../../chrome/browser/services/gcm/gcm_profile_service_unittest.cc:function > gcm::GCMProfileServiceTest::SetUp():error: undefined reference to > 'chromeos::DBusThreadManagerSetter::~DBusThreadManagerSetter()' > > > You could use an #if defined(OS_CHROMEOS) here or you could move the > ChromeOSGCMConnectionObserver into chrome/browser/chromeos/net and have it be > owned by ChromeBrowserMainPartsChromeos. My preference is for the latter since > then you probably wouldn't need to init the DBusThreadManager here. Thank you Chirantan. I am tempted to use #ifdef OS_CHROMEOS instead of moving that class one more time.
On 2014/09/12 01:24:21, Luigi Semenzato wrote: > https://codereview.chromium.org/409883006/diff/200001/chrome/browser/services... > File chrome/browser/services/gcm/gcm_profile_service_unittest.cc (right): > > https://codereview.chromium.org/409883006/diff/200001/chrome/browser/services... > chrome/browser/services/gcm/gcm_profile_service_unittest.cc:126: (void) > chromeos::DBusThreadManager::GetSetterForTesting(); > On 2014/09/12 00:52:16, chirantan wrote: > > This is causing the build to fail on non-chrome os systems: > > > > > obj/chrome/browser/services/gcm/unit_tests.gcm_profile_service_unittest.o:../../chrome/browser/services/gcm/gcm_profile_service_unittest.cc:function > > gcm::GCMProfileServiceTest::SetUp():error: undefined reference to > > 'chromeos::DBusThreadManager::GetSetterForTesting()' > > > obj/chrome/browser/services/gcm/unit_tests.gcm_profile_service_unittest.o:../../chrome/browser/services/gcm/gcm_profile_service_unittest.cc:function > > gcm::GCMProfileServiceTest::SetUp():error: undefined reference to > > 'chromeos::DBusThreadManagerSetter::~DBusThreadManagerSetter()' > > > > > > You could use an #if defined(OS_CHROMEOS) here or you could move the > > ChromeOSGCMConnectionObserver into chrome/browser/chromeos/net and have it be > > owned by ChromeBrowserMainPartsChromeos. My preference is for the latter > since > > then you probably wouldn't need to init the DBusThreadManager here. > > Thank you Chirantan. I am tempted to use #ifdef OS_CHROMEOS instead of moving > that class one more time. Actually, I am not sure it's possible to follow your suggestion. The connection observer has to be created in the context of a profile.
The CQ bit was checked by semenzato@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/409883006/220001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_compile_dbg on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
On 2014/09/12 02:44:43, I haz the power (commit-bot) wrote: > Try jobs failed on following builders: > win_chromium_compile_dbg on tryserver.chromium.win > (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) I looked all over the log file and cannot explain the failure. Was it an infrastructure problem? Thanks!
The CQ bit was checked by semenzato@chromium.org
On 2014/09/12 15:10:01, Luigi Semenzato wrote: > On 2014/09/12 02:44:43, I haz the power (commit-bot) wrote: > > Try jobs failed on following builders: > > win_chromium_compile_dbg on tryserver.chromium.win > > > (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) > > I looked all over the log file and cannot explain the failure. Was it an > infrastructure problem? Thanks! Giving it another try, just in case.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/409883006/220001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
semenzato@chromium.org changed reviewers: + scheib@chromium.org, yoz@chromium.org
Vincent and/or Yoyo, would you please review a small change to extension_gcm_app_handler_unittest.cc? (First file.) You are listed as OWNERS. Essentially to run these unit tests on "Linux-pretending-to-be-ChromeOS" we need to set up some state (a dbus manager thread, I think). I realize it would be better to do without the #ifdef, but I've noticed it's already used for similar reasons. Thanks! Luigi
On 2014/09/13 00:42:50, Luigi Semenzato wrote: > Vincent and/or Yoyo, would you please review a small change to > extension_gcm_app_handler_unittest.cc? (First file.) You are listed as OWNERS. > > Essentially to run these unit tests on "Linux-pretending-to-be-ChromeOS" we need > to set up some state (a dbus manager thread, I think). I realize it would be > better to do without the #ifdef, but I've noticed it's already used for similar > reasons. > > Thanks! > Luigi LGTM for owners
https://codereview.chromium.org/409883006/diff/240001/chrome/browser/extensio... File chrome/browser/extensions/extension_gcm_app_handler_unittest.cc (right): https://codereview.chromium.org/409883006/diff/240001/chrome/browser/extensio... chrome/browser/extensions/extension_gcm_app_handler_unittest.cc:225: (void) chromeos::DBusThreadManager::GetSetterForTesting(); What is this (void) doing here?
https://codereview.chromium.org/409883006/diff/240001/chrome/browser/extensio... File chrome/browser/extensions/extension_gcm_app_handler_unittest.cc (right): https://codereview.chromium.org/409883006/diff/240001/chrome/browser/extensio... chrome/browser/extensions/extension_gcm_app_handler_unittest.cc:225: (void) chromeos::DBusThreadManager::GetSetterForTesting(); On 2014/09/13 00:50:47, Yoyo Zhou wrote: > What is this (void) doing here? The function returns a value which I ignore, as I thought was clear from the comment (obviously it is not). Should I omit the (void)?
https://codereview.chromium.org/409883006/diff/240001/chrome/browser/extensio... File chrome/browser/extensions/extension_gcm_app_handler_unittest.cc (right): https://codereview.chromium.org/409883006/diff/240001/chrome/browser/extensio... chrome/browser/extensions/extension_gcm_app_handler_unittest.cc:225: (void) chromeos::DBusThreadManager::GetSetterForTesting(); On 2014/09/13 01:02:48, Luigi Semenzato wrote: > On 2014/09/13 00:50:47, Yoyo Zhou wrote: > > What is this (void) doing here? > > The function returns a value which I ignore, as I thought was clear from the > comment (obviously it is not). Should I omit the (void)? The usage looks pretty uncommon in src/chrome, so I would say you can omit it.
On 2014/09/13 01:09:43, Yoyo Zhou wrote: > https://codereview.chromium.org/409883006/diff/240001/chrome/browser/extensio... > File chrome/browser/extensions/extension_gcm_app_handler_unittest.cc (right): > > https://codereview.chromium.org/409883006/diff/240001/chrome/browser/extensio... > chrome/browser/extensions/extension_gcm_app_handler_unittest.cc:225: (void) > chromeos::DBusThreadManager::GetSetterForTesting(); > On 2014/09/13 01:02:48, Luigi Semenzato wrote: > > On 2014/09/13 00:50:47, Yoyo Zhou wrote: > > > What is this (void) doing here? > > > > The function returns a value which I ignore, as I thought was clear from the > > comment (obviously it is not). Should I omit the (void)? > > The usage looks pretty uncommon in src/chrome, so I would say you can omit it. Thank you, removed void casts.
The CQ bit was checked by semenzato@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/409883006/260001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_compile_dbg on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by semenzato@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/409883006/280001
Message was sent while issue was closed.
Committed patchset #15 (id:280001) as 2fb40e12b3e04a626ffbe53aededb6e2a02b958e
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/88a95a70fa94bb578b371490d0f9868584b7096e Cr-Commit-Position: refs/heads/master@{#294936}
Message was sent while issue was closed.
A revert of this CL (patchset #15 id:280001) has been created in https://codereview.chromium.org/584753002/ by skuhne@chromium.org. The reason for reverting is: See issue https://code.google.com/p/chromium/issues/detail?id=415281 I bissected chrome to this patch and figured that this is causing chrome to crash. Sorry..
Message was sent while issue was closed.
Hello reviewers! This has been reverted because it caused random crashes in the BVT. All I know is they are signal 6 (SIGABORT): not a segv, rather some assertion failure. I am trying to analyze the code and have some questions: We have this code in the GCMProfileService constructor: chromeos_connection_observer_.reset(new gcm::ChromeOSGCMConnectionObserver); driver_->AddConnectionObserver(chromeos_connection_observer_.get()); and the reverse in GCMProfileService::ShutDown(): driver_->RemoveConnectionObserver(chromeos_connection_observer_.get()); chromeos_connection_observer_.reset(); Is this correct? Is it possible that ShutDown() is called multiple times on the same GCMProfileService object? Or is it the case that the connection observer should be added in Register()? Similarly, is it possible that a GCMProfileService object outlives the ShillManagerClient object? I see no other opportunities for races. Thank you! |
