Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1115083002:
Add the supporting code for the cast system tray integration (Closed)
DescriptionAdd the supporting code for the cast system tray integration
- Adds CastConfigDelegate which gathers casting data from the chromecast extension
- Provides access to the CastConfigDelegate to UI code via the SystemTrayDelegate
BUG=433140
Committed: https://crrev.com/fe2e0ce0c145d955f2aa3c04cf2f82db987fd83a
Cr-Commit-Position: refs/heads/master@{#330266}
Patch Set 1 #Patch Set 2 : Support detecting if we are casting a tab or the desktop #
Total comments: 26
Patch Set 3 : Fix nits and remove CastObserver #Patch Set 4 : Fix a few more nits #Patch Set 5 : Share chromecast extension ids with other code #
Total comments: 35
Patch Set 6 : Address change requests #
Total comments: 2
Patch Set 7 : Update comment #Patch Set 8 : Fix merge issue #Patch Set 9 : Fix build for unit tests #
Total comments: 13
Patch Set 10 : Address comments #Patch Set 11 : Address comments #
Total comments: 1
Patch Set 12 : Add reference to bug #
Messages
Total messages: 42 (11 generated)
jdufault@chromium.org changed reviewers: + achuith@chromium.org
Could you please flesh out the description to provide more guidance as to all the things happening in this CL? Use bullet points for clarity. https://codereview.chromium.org/1115083002/diff/20001/ash/cast_config_delegate.h File ash/cast_config_delegate.h (right): https://codereview.chromium.org/1115083002/diff/20001/ash/cast_config_delegat... ash/cast_config_delegate.h:32: enum TabId { Please provide an explanation of how these values came about https://codereview.chromium.org/1115083002/diff/20001/ash/cast_config_delegat... ash/cast_config_delegate.h:47: // The id for the tab we are casting. Could be one of the TabId values.:w Remove :w https://codereview.chromium.org/1115083002/diff/20001/ash/cast_config_delegat... ash/cast_config_delegate.h:48: int tab_id; should this have a default value? https://codereview.chromium.org/1115083002/diff/20001/ash/cast_config_delegat... ash/cast_config_delegate.h:61: virtual ~CastConfigDelegate() {} Let's move the implementation to the .cc as well https://codereview.chromium.org/1115083002/diff/20001/ash/system/cast/cast_ob... File ash/system/cast/cast_observer.h (right): https://codereview.chromium.org/1115083002/diff/20001/ash/system/cast/cast_ob... ash/system/cast/cast_observer.h:10: class CastObserver { Is this used for anything? https://codereview.chromium.org/1115083002/diff/20001/ash/system/tray/default... File ash/system/tray/default_system_tray_delegate.cc (left): https://codereview.chromium.org/1115083002/diff/20001/ash/system/tray/default... ash/system/tray/default_system_tray_delegate.cc:9: #include "ash/networking_config_delegate.h" Pull this file out into a separate CL https://codereview.chromium.org/1115083002/diff/20001/ash/system/tray/system_... File ash/system/tray/system_tray_delegate.h (right): https://codereview.chromium.org/1115083002/diff/20001/ash/system/tray/system_... ash/system/tray/system_tray_delegate.h:288: // Returns CastConfigDelegate. May return nullptr. Not a super useful comment but I understand it's consistent with the rest. Oh well https://codereview.chromium.org/1115083002/diff/20001/chrome/browser/ui/ash/c... File chrome/browser/ui/ash/cast_config_delegate_chromeos.cc (right): https://codereview.chromium.org/1115083002/diff/20001/chrome/browser/ui/ash/c... chrome/browser/ui/ash/cast_config_delegate_chromeos.cc:26: const char* kExtensionIds[] = { Please file a bug and add a TODO that this needs to share code with https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... Having a copy is not the best. And sadly, looks like the two have already diverged. It would be even nicer if you could do the refactoring CL that shares these constants before landing this CL, but it's not necessary. https://codereview.chromium.org/1115083002/diff/20001/chrome/browser/ui/ash/c... chrome/browser/ui/ash/cast_config_delegate_chromeos.cc:50: std::string extension_id(kExtensionIds[i]); const https://codereview.chromium.org/1115083002/diff/20001/chrome/browser/ui/ash/c... chrome/browser/ui/ash/cast_config_delegate_chromeos.cc:59: content::RenderViewHost* GetRenderViewHost() { Add fn comment https://codereview.chromium.org/1115083002/diff/20001/chrome/browser/ui/ash/c... chrome/browser/ui/ash/cast_config_delegate_chromeos.cc:72: void ExecuteJavaScriptWithCallback(const std::string& javascript, Fn comment https://codereview.chromium.org/1115083002/diff/20001/chrome/browser/ui/ash/c... chrome/browser/ui/ash/cast_config_delegate_chromeos.cc:89: * activity_dict = nullptr; This looks weird to me, maybe const base::DictionaryValue *receiver_dict(null_ptr), *activity_dict(null_ptr); https://codereview.chromium.org/1115083002/diff/20001/chrome/browser/ui/ash/c... chrome/browser/ui/ash/cast_config_delegate_chromeos.cc:127: // TODO(jdufault): why is getMirrorCapableReceiversAndActivites() exported What's this TODO?
I have removed CastObserver https://codereview.chromium.org/1115083002/diff/20001/ash/cast_config_delegate.h File ash/cast_config_delegate.h (right): https://codereview.chromium.org/1115083002/diff/20001/ash/cast_config_delegat... ash/cast_config_delegate.h:32: enum TabId { On 2015/05/06 00:45:57, achuithb wrote: > Please provide an explanation of how these values came about Done. https://codereview.chromium.org/1115083002/diff/20001/ash/cast_config_delegat... ash/cast_config_delegate.h:47: // The id for the tab we are casting. Could be one of the TabId values.:w On 2015/05/06 00:45:57, achuithb wrote: > Remove :w Done. https://codereview.chromium.org/1115083002/diff/20001/ash/cast_config_delegat... ash/cast_config_delegate.h:48: int tab_id; On 2015/05/06 00:45:57, achuithb wrote: > should this have a default value? Done. https://codereview.chromium.org/1115083002/diff/20001/ash/cast_config_delegat... ash/cast_config_delegate.h:61: virtual ~CastConfigDelegate() {} On 2015/05/06 00:45:57, achuithb wrote: > Let's move the implementation to the .cc as well Done. https://codereview.chromium.org/1115083002/diff/20001/ash/system/tray/default... File ash/system/tray/default_system_tray_delegate.cc (left): https://codereview.chromium.org/1115083002/diff/20001/ash/system/tray/default... ash/system/tray/default_system_tray_delegate.cc:9: #include "ash/networking_config_delegate.h" On 2015/05/06 00:45:57, achuithb wrote: > Pull this file out into a separate CL Done. https://codereview.chromium.org/1115083002/diff/20001/chrome/browser/ui/ash/c... File chrome/browser/ui/ash/cast_config_delegate_chromeos.cc (right): https://codereview.chromium.org/1115083002/diff/20001/chrome/browser/ui/ash/c... chrome/browser/ui/ash/cast_config_delegate_chromeos.cc:26: const char* kExtensionIds[] = { On 2015/05/06 00:45:58, achuithb wrote: > Please file a bug and add a TODO that this needs to share code with > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... > > Having a copy is not the best. And sadly, looks like the two have already > diverged. > > It would be even nicer if you could do the refactoring CL that shares these > constants before landing this CL, but it's not necessary. I can create a CL to do this refactoring, but it seems like that list of items contains more than just the chromecast extension ids. But looking more closely, it seems like the last two items here in lines 38 and 39 probably should not be in this list since they are not the actual extension itself. Even if the extension id supports/requires desktop mirroring does not mean we will be able to interface with it, since we call into the JS of the extension. https://codereview.chromium.org/1115083002/diff/20001/chrome/browser/ui/ash/c... chrome/browser/ui/ash/cast_config_delegate_chromeos.cc:50: std::string extension_id(kExtensionIds[i]); On 2015/05/06 00:45:57, achuithb wrote: > const Done. https://codereview.chromium.org/1115083002/diff/20001/chrome/browser/ui/ash/c... chrome/browser/ui/ash/cast_config_delegate_chromeos.cc:59: content::RenderViewHost* GetRenderViewHost() { On 2015/05/06 00:45:57, achuithb wrote: > Add fn comment Done. https://codereview.chromium.org/1115083002/diff/20001/chrome/browser/ui/ash/c... chrome/browser/ui/ash/cast_config_delegate_chromeos.cc:72: void ExecuteJavaScriptWithCallback(const std::string& javascript, On 2015/05/06 00:45:58, achuithb wrote: > Fn comment Done. https://codereview.chromium.org/1115083002/diff/20001/chrome/browser/ui/ash/c... chrome/browser/ui/ash/cast_config_delegate_chromeos.cc:89: * activity_dict = nullptr; On 2015/05/06 00:45:57, achuithb wrote: > This looks weird to me, maybe > const base::DictionaryValue *receiver_dict(null_ptr), *activity_dict(null_ptr); Done. https://codereview.chromium.org/1115083002/diff/20001/chrome/browser/ui/ash/c... chrome/browser/ui/ash/cast_config_delegate_chromeos.cc:127: // TODO(jdufault): why is getMirrorCapableReceiversAndActivites() exported On 2015/05/06 00:45:57, achuithb wrote: > What's this TODO? Just a note to myself, removed.
https://codereview.chromium.org/1115083002/diff/20001/chrome/browser/ui/ash/c... File chrome/browser/ui/ash/cast_config_delegate_chromeos.cc (right): https://codereview.chromium.org/1115083002/diff/20001/chrome/browser/ui/ash/c... chrome/browser/ui/ash/cast_config_delegate_chromeos.cc:26: const char* kExtensionIds[] = { On 2015/05/06 19:02:55, jdufault wrote: > On 2015/05/06 00:45:58, achuithb wrote: > > Please file a bug and add a TODO that this needs to share code with > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... > > > > Having a copy is not the best. And sadly, looks like the two have already > > diverged. > > > > It would be even nicer if you could do the refactoring CL that shares these > > constants before landing this CL, but it's not necessary. > > I can create a CL to do this refactoring, but it seems like that list of items > contains more than just the chromecast extension ids. But looking more closely, > it seems like the last two items here in lines 38 and 39 probably should not be > in this list since they are not the actual extension itself. Even if the > extension id supports/requires desktop mirroring does not mean we will be able > to interface with it, since we call into the JS of the extension. So remove lines 37-39, right? That would make the two lists consistent. I think it would be good to have one .h file that exports this list of constants to both places. I'm not sure what a good location would be. I think this should be done rather than the TODO.
jdufault@chromium.org changed reviewers: + jennyz@chromium.org
https://codereview.chromium.org/1115083002/diff/20001/chrome/browser/ui/ash/c... File chrome/browser/ui/ash/cast_config_delegate_chromeos.cc (right): https://codereview.chromium.org/1115083002/diff/20001/chrome/browser/ui/ash/c... chrome/browser/ui/ash/cast_config_delegate_chromeos.cc:26: const char* kExtensionIds[] = { On 2015/05/06 19:08:59, achuithb wrote: > On 2015/05/06 19:02:55, jdufault wrote: > > On 2015/05/06 00:45:58, achuithb wrote: > > > Please file a bug and add a TODO that this needs to share code with > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... > > > > > > Having a copy is not the best. And sadly, looks like the two have already > > > diverged. > > > > > > It would be even nicer if you could do the refactoring CL that shares these > > > constants before landing this CL, but it's not necessary. > > > > I can create a CL to do this refactoring, but it seems like that list of items > > contains more than just the chromecast extension ids. But looking more > closely, > > it seems like the last two items here in lines 38 and 39 probably should not > be > > in this list since they are not the actual extension itself. Even if the > > extension id supports/requires desktop mirroring does not mean we will be able > > to interface with it, since we call into the JS of the extension. > > So remove lines 37-39, right? That would make the two lists consistent. > > I think it would be good to have one .h file that exports this list of constants > to both places. I'm not sure what a good location would be. I think this should > be done rather than the TODO. Done, please see https://codereview.chromium.org/1129303002/
https://codereview.chromium.org/1115083002/diff/80001/ash/cast_config_delegate.h File ash/cast_config_delegate.h (right): https://codereview.chromium.org/1115083002/diff/80001/ash/cast_config_delegat... ash/cast_config_delegate.h:30: }; Add a blank line. https://codereview.chromium.org/1115083002/diff/80001/ash/cast_config_delegat... ash/cast_config_delegate.h:54: int tab_id = TabId::DESKTOP; How about initialize it in its constructor? https://codereview.chromium.org/1115083002/diff/80001/ash/cast_config_delegat... ash/cast_config_delegate.h:55: }; ditto https://codereview.chromium.org/1115083002/diff/80001/ash/cast_config_delegat... ash/cast_config_delegate.h:62: }; ditto https://codereview.chromium.org/1115083002/diff/80001/ash/cast_config_delegat... ash/cast_config_delegate.h:63: using ReceiversAndActivites = std::map<std::string, ReceiverAndActivity>; Use typedef instead of using? Also, can you add a comment to explain what is the key of the map? https://codereview.chromium.org/1115083002/diff/80001/ash/cast_config_delegat... ash/cast_config_delegate.h:65: base::Callback<void(const ReceiversAndActivites&)>; typedef instead? https://codereview.chromium.org/1115083002/diff/80001/ash/cast_config_delegat... ash/cast_config_delegate.h:71: // Returns the list of cast receivers and activities. The function does not return list of the cast receives and activities. Please be accurate to describe how they are passed with callback. See some examples in this file: https://code.google.com/p/chromium/codesearch#chromium/src/device/bluetooth/b... https://codereview.chromium.org/1115083002/diff/80001/ash/cast_config_delegat... ash/cast_config_delegate.h:74: // Cast to a device. Please explain what |receiver_id| specifies. https://codereview.chromium.org/1115083002/diff/80001/ash/cast_config_delegat... ash/cast_config_delegate.h:76: // Stop ongoing cast. ditto. https://codereview.chromium.org/1115083002/diff/80001/chrome/browser/ui/ash/c... File chrome/browser/ui/ash/cast_config_delegate_chromeos.cc (right): https://codereview.chromium.org/1115083002/diff/80001/chrome/browser/ui/ash/c... chrome/browser/ui/ash/cast_config_delegate_chromeos.cc:24: content::RenderFrameHost::JavaScriptResultCallback; Just "using content::RenderFrameHost::JavaScriptResultCallback" is enough. https://codereview.chromium.org/1115083002/diff/80001/chrome/browser/ui/ash/c... chrome/browser/ui/ash/cast_config_delegate_chromeos.cc:47: Profile* profile = ProfileManager::GetActiveUserProfile(); Do you need to check if profile is nullptr? https://codereview.chromium.org/1115083002/diff/80001/chrome/browser/ui/ash/c... chrome/browser/ui/ash/cast_config_delegate_chromeos.cc:49: return pm->GetBackgroundHostForExtension(extension->id())->render_view_host(); Will extension ever possible be nullptr? https://codereview.chromium.org/1115083002/diff/80001/chrome/browser/ui/ash/c... File chrome/browser/ui/ash/cast_config_delegate_chromeos.h (right): https://codereview.chromium.org/1115083002/diff/80001/chrome/browser/ui/ash/c... chrome/browser/ui/ash/cast_config_delegate_chromeos.h:14: #include "content/public/browser/browser_context.h" Not used in this .h file, should be moved to .cc file. https://codereview.chromium.org/1115083002/diff/80001/chrome/browser/ui/ash/c... chrome/browser/ui/ash/cast_config_delegate_chromeos.h:15: #include "content/public/browser/render_frame_host.h" ditto
https://codereview.chromium.org/1115083002/diff/80001/ash/cast_config_delegate.h File ash/cast_config_delegate.h (right): https://codereview.chromium.org/1115083002/diff/80001/ash/cast_config_delegat... ash/cast_config_delegate.h:30: }; On 2015/05/07 21:50:34, jennyz wrote: > Add a blank line. Done. https://codereview.chromium.org/1115083002/diff/80001/ash/cast_config_delegat... ash/cast_config_delegate.h:54: int tab_id = TabId::DESKTOP; On 2015/05/07 21:50:34, jennyz wrote: > How about initialize it in its constructor? Why? https://codereview.chromium.org/1115083002/diff/80001/ash/cast_config_delegat... ash/cast_config_delegate.h:55: }; On 2015/05/07 21:50:34, jennyz wrote: > ditto Why? https://codereview.chromium.org/1115083002/diff/80001/ash/cast_config_delegat... ash/cast_config_delegate.h:62: }; On 2015/05/07 21:50:35, jennyz wrote: > ditto Done. https://codereview.chromium.org/1115083002/diff/80001/ash/cast_config_delegat... ash/cast_config_delegate.h:63: using ReceiversAndActivites = std::map<std::string, ReceiverAndActivity>; On 2015/05/07 21:50:35, jennyz wrote: > Use typedef instead of using? Also, can you add a comment to explain what is the > key of the map? Done for comment. Style guide says that using is preferred over typedef. https://codereview.chromium.org/1115083002/diff/80001/ash/cast_config_delegat... ash/cast_config_delegate.h:65: base::Callback<void(const ReceiversAndActivites&)>; On 2015/05/07 21:50:34, jennyz wrote: > typedef instead? See above. https://codereview.chromium.org/1115083002/diff/80001/ash/cast_config_delegat... ash/cast_config_delegate.h:71: // Returns the list of cast receivers and activities. On 2015/05/07 21:50:35, jennyz wrote: > The function does not return list of the cast receives and activities. Please be > accurate to describe how they are passed with callback. See some examples in > this file: > https://code.google.com/p/chromium/codesearch#chromium/src/device/bluetooth/b... Done. https://codereview.chromium.org/1115083002/diff/80001/ash/cast_config_delegat... ash/cast_config_delegate.h:74: // Cast to a device. On 2015/05/07 21:50:35, jennyz wrote: > Please explain what |receiver_id| specifies. Done. https://codereview.chromium.org/1115083002/diff/80001/ash/cast_config_delegat... ash/cast_config_delegate.h:76: // Stop ongoing cast. On 2015/05/07 21:50:34, jennyz wrote: > ditto. Done. https://codereview.chromium.org/1115083002/diff/80001/chrome/browser/ui/ash/c... File chrome/browser/ui/ash/cast_config_delegate_chromeos.cc (right): https://codereview.chromium.org/1115083002/diff/80001/chrome/browser/ui/ash/c... chrome/browser/ui/ash/cast_config_delegate_chromeos.cc:24: content::RenderFrameHost::JavaScriptResultCallback; On 2015/05/07 21:50:35, jennyz wrote: > Just "using content::RenderFrameHost::JavaScriptResultCallback" is enough. Compile error :( https://codereview.chromium.org/1115083002/diff/80001/chrome/browser/ui/ash/c... chrome/browser/ui/ash/cast_config_delegate_chromeos.cc:47: Profile* profile = ProfileManager::GetActiveUserProfile(); On 2015/05/07 21:50:35, jennyz wrote: > Do you need to check if profile is nullptr? Done. https://codereview.chromium.org/1115083002/diff/80001/chrome/browser/ui/ash/c... chrome/browser/ui/ash/cast_config_delegate_chromeos.cc:49: return pm->GetBackgroundHostForExtension(extension->id())->render_view_host(); On 2015/05/07 21:50:35, jennyz wrote: > Will extension ever possible be nullptr? Done. https://codereview.chromium.org/1115083002/diff/80001/chrome/browser/ui/ash/c... File chrome/browser/ui/ash/cast_config_delegate_chromeos.h (right): https://codereview.chromium.org/1115083002/diff/80001/chrome/browser/ui/ash/c... chrome/browser/ui/ash/cast_config_delegate_chromeos.h:14: #include "content/public/browser/browser_context.h" On 2015/05/07 21:50:35, jennyz wrote: > Not used in this .h file, should be moved to .cc file. Done. https://codereview.chromium.org/1115083002/diff/80001/chrome/browser/ui/ash/c... chrome/browser/ui/ash/cast_config_delegate_chromeos.h:15: #include "content/public/browser/render_frame_host.h" On 2015/05/07 21:50:35, jennyz wrote: > ditto Done.
https://codereview.chromium.org/1115083002/diff/80001/ash/cast_config_delegate.h File ash/cast_config_delegate.h (right): https://codereview.chromium.org/1115083002/diff/80001/ash/cast_config_delegat... ash/cast_config_delegate.h:54: int tab_id = TabId::DESKTOP; On 2015/05/07 23:41:52, jdufault wrote: > On 2015/05/07 21:50:34, jennyz wrote: > > How about initialize it in its constructor? > > Why? I typically looks into constructor for the default values of the class members. Maybe this is just my preference. No big deal if you prefer the other way. https://codereview.chromium.org/1115083002/diff/80001/ash/cast_config_delegat... ash/cast_config_delegate.h:55: }; On 2015/05/07 23:41:53, jdufault wrote: > On 2015/05/07 21:50:34, jennyz wrote: > > ditto > > Why? It looks neat between two structs. https://codereview.chromium.org/1115083002/diff/80001/ash/cast_config_delegat... ash/cast_config_delegate.h:63: using ReceiversAndActivites = std::map<std::string, ReceiverAndActivity>; On 2015/05/07 23:41:52, jdufault wrote: > On 2015/05/07 21:50:35, jennyz wrote: > > Use typedef instead of using? Also, can you add a comment to explain what is > the > > key of the map? > > Done for comment. > > Style guide says that using is preferred over typedef. Can you point me the Google or chromium c++ coding style guideline for this recommendation? https://codereview.chromium.org/1115083002/diff/100001/ash/cast_config_delega... File ash/cast_config_delegate.h (right): https://codereview.chromium.org/1115083002/diff/100001/ash/cast_config_delega... ash/cast_config_delegate.h:81: // we can cast to, ie, a Chromecast. I didn't mean you need to explain what |receiver_id| is, but explain what it refers to. How about something like: "Cast to a receiver specified by |receiver_id|."?
https://codereview.chromium.org/1115083002/diff/80001/ash/cast_config_delegate.h File ash/cast_config_delegate.h (right): https://codereview.chromium.org/1115083002/diff/80001/ash/cast_config_delegat... ash/cast_config_delegate.h:63: using ReceiversAndActivites = std::map<std::string, ReceiverAndActivity>; On 2015/05/08 18:01:15, jennyz wrote: > On 2015/05/07 23:41:52, jdufault wrote: > > On 2015/05/07 21:50:35, jennyz wrote: > > > Use typedef instead of using? Also, can you add a comment to explain what is > > the > > > key of the map? > > > > Done for comment. > > > > Style guide says that using is preferred over typedef. > > Can you point me the Google or chromium c++ coding style guideline for this > recommendation? Jenny, using instead of a typedef for type alias is a new C++11 feature, and it is preferred. Here's the discussion: https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/8dOAMzgR4ao It's the first item in the list here: https://chromium-cpp.appspot.com/
https://codereview.chromium.org/1115083002/diff/80001/ash/cast_config_delegate.h File ash/cast_config_delegate.h (right): https://codereview.chromium.org/1115083002/diff/80001/ash/cast_config_delegat... ash/cast_config_delegate.h:54: int tab_id = TabId::DESKTOP; On 2015/05/08 18:01:15, jennyz wrote: > On 2015/05/07 23:41:52, jdufault wrote: > > On 2015/05/07 21:50:34, jennyz wrote: > > > How about initialize it in its constructor? > > > > Why? > > I typically looks into constructor for the default values of the class members. > Maybe this is just my preference. No big deal if you prefer the other way. This has also changed with C++11. We're now supposed to prefer in-class member initialization to initialization in the ctor. Discussion is here: https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/zqB-DySA4V0 And the style guide recommends it here: https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Initialization The rationale is that with no ctor, or multiple ctors, there's a chance that some variables could be left uninitialized.
lgtm https://codereview.chromium.org/1115083002/diff/80001/ash/cast_config_delegate.h File ash/cast_config_delegate.h (right): https://codereview.chromium.org/1115083002/diff/80001/ash/cast_config_delegat... ash/cast_config_delegate.h:54: int tab_id = TabId::DESKTOP; On 2015/05/08 18:19:44, achuithb wrote: > On 2015/05/08 18:01:15, jennyz wrote: > > On 2015/05/07 23:41:52, jdufault wrote: > > > On 2015/05/07 21:50:34, jennyz wrote: > > > > How about initialize it in its constructor? > > > > > > Why? > > > > I typically looks into constructor for the default values of the class > members. > > Maybe this is just my preference. No big deal if you prefer the other way. > > This has also changed with C++11. We're now supposed to prefer in-class member > initialization to initialization in the ctor. Discussion is here: > https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/zqB-DySA4V0 > > And the style guide recommends it here: > https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Initialization > > The rationale is that with no ctor, or multiple ctors, there's a chance that > some variables could be left uninitialized. Acknowledged. https://codereview.chromium.org/1115083002/diff/80001/ash/cast_config_delegat... ash/cast_config_delegate.h:63: using ReceiversAndActivites = std::map<std::string, ReceiverAndActivity>; On 2015/05/08 18:12:19, achuithb wrote: > On 2015/05/08 18:01:15, jennyz wrote: > > On 2015/05/07 23:41:52, jdufault wrote: > > > On 2015/05/07 21:50:35, jennyz wrote: > > > > Use typedef instead of using? Also, can you add a comment to explain what > is > > > the > > > > key of the map? > > > > > > Done for comment. > > > > > > Style guide says that using is preferred over typedef. > > > > Can you point me the Google or chromium c++ coding style guideline for this > > recommendation? > > Jenny, using instead of a typedef for type alias is a new C++11 feature, and it > is preferred. Here's the discussion: > https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/8dOAMzgR4ao > > It's the first item in the list here: > https://chromium-cpp.appspot.com/ Acknowledged.
https://codereview.chromium.org/1115083002/diff/100001/ash/cast_config_delega... File ash/cast_config_delegate.h (right): https://codereview.chromium.org/1115083002/diff/100001/ash/cast_config_delega... ash/cast_config_delegate.h:81: // we can cast to, ie, a Chromecast. On 2015/05/08 18:01:16, jennyz wrote: > I didn't mean you need to explain what |receiver_id| is, but explain what it > refers to. How about something like: "Cast to a receiver specified by > |receiver_id|."? Done.
The CQ bit was checked by jdufault@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jennyz@chromium.org Link to the patchset: https://codereview.chromium.org/1115083002/#ps120001 (title: "Update comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1115083002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
The CQ bit was checked by jdufault@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jennyz@chromium.org Link to the patchset: https://codereview.chromium.org/1115083002/#ps140001 (title: "Fix merge issue")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1115083002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
achuith@chromium.org changed reviewers: + oshima@chromium.org
Oshima-san, need owner lgtm.
can you check link error on dbg build?
On 2015/05/14 00:22:28, oshima wrote: > can you check link error on dbg build? It should be working now.
https://codereview.chromium.org/1115083002/diff/160001/ash/cast_config_delega... File ash/cast_config_delegate.h (right): https://codereview.chromium.org/1115083002/diff/160001/ash/cast_config_delega... ash/cast_config_delegate.h:32: struct ASH_EXPORT Activity { ASH_EXPORT struct is more common, although you may leave it if you prefer (I don't have strong opinion) https://codereview.chromium.org/1115083002/diff/160001/ash/cast_config_delega... ash/cast_config_delegate.h:73: virtual bool HasCastExtension() = 0; can this be const? https://codereview.chromium.org/1115083002/diff/160001/ash/cast_config_delega... ash/cast_config_delegate.h:73: virtual bool HasCastExtension() = 0; new line between previous method and comment. same for the rest. https://codereview.chromium.org/1115083002/diff/160001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/cast_config_delegate_chromeos.cc (right): https://codereview.chromium.org/1115083002/diff/160001/chrome/browser/ui/ash/... chrome/browser/ui/ash/cast_config_delegate_chromeos.cc:30: Profile* profile = ProfileManager::GetActiveUserProfile(); Instead of using active user profile, it's probably better topass the browser context for the user who'll use cast. https://codereview.chromium.org/1115083002/diff/160001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/cast_config_delegate_chromeos.h (right): https://codereview.chromium.org/1115083002/diff/160001/chrome/browser/ui/ash/... chrome/browser/ui/ash/cast_config_delegate_chromeos.h:25: // CastConfigDelegate overrides. This is old style. new style is // CastConfigDelegate:
I have hopefully addressed all of the comments. I haven't had a chance to test on-device due to deployment issues, but I expect everything should work as expected. https://codereview.chromium.org/1115083002/diff/160001/ash/cast_config_delega... File ash/cast_config_delegate.h (right): https://codereview.chromium.org/1115083002/diff/160001/ash/cast_config_delega... ash/cast_config_delegate.h:32: struct ASH_EXPORT Activity { On 2015/05/14 23:05:29, oshima wrote: > ASH_EXPORT struct is more common, although you may leave it if you prefer (I > don't have strong opinion) This causes a compiler error. https://codereview.chromium.org/1115083002/diff/160001/ash/cast_config_delega... ash/cast_config_delegate.h:73: virtual bool HasCastExtension() = 0; On 2015/05/14 23:05:29, oshima wrote: > new line between previous method and comment. same for the rest. Done. https://codereview.chromium.org/1115083002/diff/160001/ash/cast_config_delega... ash/cast_config_delegate.h:73: virtual bool HasCastExtension() = 0; On 2015/05/14 23:05:29, oshima wrote: > can this be const? Done. https://codereview.chromium.org/1115083002/diff/160001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/cast_config_delegate_chromeos.cc (right): https://codereview.chromium.org/1115083002/diff/160001/chrome/browser/ui/ash/... chrome/browser/ui/ash/cast_config_delegate_chromeos.cc:30: Profile* profile = ProfileManager::GetActiveUserProfile(); On 2015/05/14 23:05:29, oshima wrote: > Instead of using active user profile, it's probably better topass the browser > context for the user who'll use cast. I've changed it to GetPrimaryUserProfile() which should avoid the issues with multi-user login. I can thread the profile out as a parameter, but then every call site is going to have to specify which profile/browsercontext to use (which should all be GetPrimaryUserProfile()). https://codereview.chromium.org/1115083002/diff/160001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/cast_config_delegate_chromeos.h (right): https://codereview.chromium.org/1115083002/diff/160001/chrome/browser/ui/ash/... chrome/browser/ui/ash/cast_config_delegate_chromeos.h:25: // CastConfigDelegate overrides. On 2015/05/14 23:05:29, oshima wrote: > This is old style. new style is > > // CastConfigDelegate: > Done.
https://codereview.chromium.org/1115083002/diff/160001/ash/cast_config_delega... File ash/cast_config_delegate.h (right): https://codereview.chromium.org/1115083002/diff/160001/ash/cast_config_delega... ash/cast_config_delegate.h:32: struct ASH_EXPORT Activity { On 2015/05/15 23:02:25, jdufault wrote: > On 2015/05/14 23:05:29, oshima wrote: > > ASH_EXPORT struct is more common, although you may leave it if you prefer (I > > don't have strong opinion) > > This causes a compiler error. sorry, I thought I removed this comment. you're right that I was wrong. https://codereview.chromium.org/1115083002/diff/160001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/cast_config_delegate_chromeos.cc (right): https://codereview.chromium.org/1115083002/diff/160001/chrome/browser/ui/ash/... chrome/browser/ui/ash/cast_config_delegate_chromeos.cc:30: Profile* profile = ProfileManager::GetActiveUserProfile(); On 2015/05/15 23:02:25, jdufault wrote: > On 2015/05/14 23:05:29, oshima wrote: > > Instead of using active user profile, it's probably better topass the browser > > context for the user who'll use cast. > > I've changed it to GetPrimaryUserProfile() which should avoid the issues with > multi-user login. > > I can thread the profile out as a parameter, but then every > call site is going to have to specify which profile/browsercontext to use (which > should all be GetPrimaryUserProfile()). That means that all users in multi profile mode will use the primary user profile, which I doubt is what we want. You're also mixing Primary and Active profile which will not work when a user switched to 2nd user. (and please see the skuhne@'s comment in profile_manager.h for these method) If you're sure that using active profile won't cause the issue, then you may land it, although you should fix this soon. Even if this may not cause an issue now, but this will be an potential risk for regression. (say you a tab casting wants to use this UI) If you don't want to pass around context, you can pass context to GetCastDelegate method, and create delegates for each context, then store it inside context (context is KeyedService).
https://codereview.chromium.org/1115083002/diff/160001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/cast_config_delegate_chromeos.cc (right): https://codereview.chromium.org/1115083002/diff/160001/chrome/browser/ui/ash/... chrome/browser/ui/ash/cast_config_delegate_chromeos.cc:30: Profile* profile = ProfileManager::GetActiveUserProfile(); On 2015/05/15 23:32:55, oshima wrote: > On 2015/05/15 23:02:25, jdufault wrote: > > On 2015/05/14 23:05:29, oshima wrote: > > > Instead of using active user profile, it's probably better topass the > browser > > > context for the user who'll use cast. > > > > I've changed it to GetPrimaryUserProfile() which should avoid the issues with > > multi-user login. > > > > I can thread the profile out as a parameter, but then every > > call site is going to have to specify which profile/browsercontext to use > (which > > should all be GetPrimaryUserProfile()). > > That means that all users in multi profile mode will use the primary user > profile, > which I doubt is what we want. You're also mixing Primary and Active profile > which will > not work when a user switched to 2nd user. (and please see the skuhne@'s comment > in > profile_manager.h for these method) > > If you're sure that using active profile won't cause the issue, then you may > land it, > although you should fix this soon. Even if this may not cause an issue now, but > this will > be an potential risk for regression. (say you a tab casting wants to use this > UI) > > If you don't want to pass around context, you can pass context to > GetCastDelegate method, > and create delegates for each context, then store it inside context (context is > KeyedService). Reverted for now, will address multi-profile behavior pending design talks.
On 2015/05/15 23:58:27, jdufault wrote: > https://codereview.chromium.org/1115083002/diff/160001/chrome/browser/ui/ash/... > File chrome/browser/ui/ash/cast_config_delegate_chromeos.cc (right): > > https://codereview.chromium.org/1115083002/diff/160001/chrome/browser/ui/ash/... > chrome/browser/ui/ash/cast_config_delegate_chromeos.cc:30: Profile* profile = > ProfileManager::GetActiveUserProfile(); > On 2015/05/15 23:32:55, oshima wrote: > > On 2015/05/15 23:02:25, jdufault wrote: > > > On 2015/05/14 23:05:29, oshima wrote: > > > > Instead of using active user profile, it's probably better topass the > > browser > > > > context for the user who'll use cast. > > > > > > I've changed it to GetPrimaryUserProfile() which should avoid the issues > with > > > multi-user login. > > > > > > I can thread the profile out as a parameter, but then every > > > call site is going to have to specify which profile/browsercontext to use > > (which > > > should all be GetPrimaryUserProfile()). > > > > That means that all users in multi profile mode will use the primary user > > profile, > > which I doubt is what we want. You're also mixing Primary and Active profile > > which will > > not work when a user switched to 2nd user. (and please see the skuhne@'s > comment > > in > > profile_manager.h for these method) > > > > If you're sure that using active profile won't cause the issue, then you may > > land it, > > although you should fix this soon. Even if this may not cause an issue now, > but > > this will > > be an potential risk for regression. (say you a tab casting wants to use this > > UI) > > > > If you don't want to pass around context, you can pass context to > > GetCastDelegate method, > > and create delegates for each context, then store it inside context (context > is > > KeyedService). > > Reverted for now, will address multi-profile behavior pending design talks. Did you forget to upload?
On 2015/05/16 00:00:48, achuithb wrote: > On 2015/05/15 23:58:27, jdufault wrote: > > > https://codereview.chromium.org/1115083002/diff/160001/chrome/browser/ui/ash/... > > File chrome/browser/ui/ash/cast_config_delegate_chromeos.cc (right): > > > > > https://codereview.chromium.org/1115083002/diff/160001/chrome/browser/ui/ash/... > > chrome/browser/ui/ash/cast_config_delegate_chromeos.cc:30: Profile* profile = > > ProfileManager::GetActiveUserProfile(); > > On 2015/05/15 23:32:55, oshima wrote: > > > On 2015/05/15 23:02:25, jdufault wrote: > > > > On 2015/05/14 23:05:29, oshima wrote: > > > > > Instead of using active user profile, it's probably better topass the > > > browser > > > > > context for the user who'll use cast. > > > > > > > > I've changed it to GetPrimaryUserProfile() which should avoid the issues > > with > > > > multi-user login. > > > > > > > > I can thread the profile out as a parameter, but then every > > > > call site is going to have to specify which profile/browsercontext to use > > > (which > > > > should all be GetPrimaryUserProfile()). > > > > > > That means that all users in multi profile mode will use the primary user > > > profile, > > > which I doubt is what we want. You're also mixing Primary and Active profile > > > which will > > > not work when a user switched to 2nd user. (and please see the skuhne@'s > > comment > > > in > > > profile_manager.h for these method) > > > > > > If you're sure that using active profile won't cause the issue, then you may > > > land it, > > > although you should fix this soon. Even if this may not cause an issue now, > > but > > > this will > > > be an potential risk for regression. (say you a tab casting wants to use > this > > > UI) > > > > > > If you don't want to pass around context, you can pass context to > > > GetCastDelegate method, > > > and create delegates for each context, then store it inside context (context > > is > > > KeyedService). > > > > Reverted for now, will address multi-profile behavior pending design talks. > > Did you forget to upload? Oops, sorry. Should be done now.
https://codereview.chromium.org/1115083002/diff/200001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/cast_config_delegate_chromeos.cc (right): https://codereview.chromium.org/1115083002/diff/200001/chrome/browser/ui/ash/... chrome/browser/ui/ash/cast_config_delegate_chromeos.cc:30: // TODO(jdufault): Figure out which profile to use when in multiprofile mode. Please also file a bug and reference it here.
On 2015/05/16 00:10:47, achuithb wrote: > https://codereview.chromium.org/1115083002/diff/200001/chrome/browser/ui/ash/... > File chrome/browser/ui/ash/cast_config_delegate_chromeos.cc (right): > > https://codereview.chromium.org/1115083002/diff/200001/chrome/browser/ui/ash/... > chrome/browser/ui/ash/cast_config_delegate_chromeos.cc:30: // TODO(jdufault): > Figure out which profile to use when in multiprofile mode. > Please also file a bug and reference it here. Done
lgtm
The CQ bit was checked by jdufault@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from achuith@chromium.org, jennyz@chromium.org Link to the patchset: https://codereview.chromium.org/1115083002/#ps220001 (title: "Add reference to bug")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1115083002/220001
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/fe2e0ce0c145d955f2aa3c04cf2f82db987fd83a Cr-Commit-Position: refs/heads/master@{#330266} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
