Rename AppsClient -> AppWindowClient

Rename AppsClient -> AppWindowClient

per the comment in apps_client.h

BUG=None
TBR=sky@chromium.org

Committed: https://crrev.com/8304b516b8d4463493fd0055a7f2e03e2d182838
Cr-Commit-Position: refs/heads/master@{#295671}

[oshima, 2014-09-18 21:30:01]

oshima@chromium.org changed reviewers:
+ hashimoto@chromium.org, jamescook@chromium.org

[oshima, 2014-09-18 21:38:24]

Patchset #1 (id:1) has been deleted

[oshima, 2014-09-18 21:39:33]

https://codereview.chromium.org/583583008/diff/20001/extensions/browser/app_w...
File extensions/browser/app_window/app_window_client.h (right):

https://codereview.chromium.org/583583008/diff/20001/extensions/browser/app_w...
extensions/browser/app_window/app_window_client.h:25: // TODO(hashimoto): Move
some functions to ExtensionsClient.
I'll leave this to hashimoto@ :)

[James Cook, 2014-09-18 22:29:13]

LGTM

https://codereview.chromium.org/583583008/diff/20001/extensions/browser/app_w...
File extensions/browser/app_window/app_window_client.h (right):

https://codereview.chromium.org/583583008/diff/20001/extensions/browser/app_w...
extensions/browser/app_window/app_window_client.h:25: // TODO(hashimoto): Move
some functions to ExtensionsClient.
On 2014/09/18 21:39:32, oshima wrote:
> I'll leave this to hashimoto@ :)

SGTM

GetLoadedBrowserContexts() should move to ExtensionsBrowserClient

IsCurrentChannelOlderThanDev() is not a good API function. I think we need
something that takes a channel string, like AllowFeatureFromChannel(const
std::string& feature_channel). This should live in ExtensionsClient.

hashimoto, do you have time to do this?  If you are busy, assign a bug to me and
I'll do it.

[oshima, 2014-09-18 22:53:52]

oshima@chromium.org changed reviewers:
+ sky@chromium.org

[oshima, 2014-09-18 22:54:19]

TBR'ing sky for trival class rename under chrome/

[oshima, 2014-09-18 23:14:18]

The CQ bit was checked by oshima@chromium.org

[commit-bot: I haz the power, 2014-09-18 23:16:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/583583008/40001

[commit-bot: I haz the power, 2014-09-18 23:39:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-18 23:39:10]

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_...)
  linux_chromium_gn_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[hashimoto, 2014-09-19 02:44:38]

lgtm
Please also update variables' names and comments.

https://codereview.chromium.org/583583008/diff/20001/extensions/browser/app_w...
File extensions/browser/app_window/app_window_client.h (right):

https://codereview.chromium.org/583583008/diff/20001/extensions/browser/app_w...
extensions/browser/app_window/app_window_client.h:25: // TODO(hashimoto): Move
some functions to ExtensionsClient.
On 2014/09/18 22:29:12, James Cook wrote:
> On 2014/09/18 21:39:32, oshima wrote:
> > I'll leave this to hashimoto@ :)
> 
> SGTM
> 
> GetLoadedBrowserContexts() should move to ExtensionsBrowserClient
GetLoadedBrowserContexts() is used only by the following static methods of
AppWindowRegistry.
-AppWindowRegistry::GetAppWindowForNativeWindowAnyProfile
-AppWindowRegistry::IsAppWindowRegisteredInAnyProfile
-AppWindowRegistry::CloseAllAppWindows
Since all of them are used only by chrome, I began to think these static methods
should be split from AppWindowRegistry and put in somewhere (e.g.
src/apps/app_window_registry_util.{cc,h}) so that we can get rid of
GetLoadedBrowserContexts() from the client at all.
Do you think GetLoadedBrowserContexts() can be useful for binaries other than
chrome (e.g. app_shell and athena)?
> 
> IsCurrentChannelOlderThanDev() is not a good API function. I think we need
> something that takes a channel string, like AllowFeatureFromChannel(const
> std::string& feature_channel). This should live in ExtensionsClient.
Is it OK to mention the idea of "channels" in extensions code?
If it's OK, then maybe we should move
chrome/common/extensions/features/feature_channel.h to src/extensions?
> 
> hashimoto, do you have time to do this?  If you are busy, assign a bug to me
and
> I'll do it.

https://codereview.chromium.org/583583008/diff/60001/athena/extensions/chrome...
File athena/extensions/chrome/extensions_delegate_impl.cc (right):

https://codereview.chromium.org/583583008/diff/60001/athena/extensions/chrome...
athena/extensions/chrome/extensions_delegate_impl.cc:111:
AthenaChromeAppWindowClient apps_client_;
Also rename this variable?

https://codereview.chromium.org/583583008/diff/60001/athena/extensions/shell/...
File athena/extensions/shell/extensions_delegate_impl.cc (right):

https://codereview.chromium.org/583583008/diff/60001/athena/extensions/shell/...
athena/extensions/shell/extensions_delegate_impl.cc:50: 
Also rename this variable?

https://codereview.chromium.org/583583008/diff/60001/extensions/browser/app_w...
File extensions/browser/app_window/app_window.cc (right):

https://codereview.chromium.org/583583008/diff/60001/extensions/browser/app_w...
extensions/browser/app_window/app_window.cc:275: AppWindowClient* apps_client =
AppWindowClient::Get();
Rename this variable?

https://codereview.chromium.org/583583008/diff/60001/extensions/browser/app_w...
File extensions/browser/app_window/app_window_client.h (right):

https://codereview.chromium.org/583583008/diff/60001/extensions/browser/app_w...
extensions/browser/app_window/app_window_client.h:23: // Sets up global state
for the apps system. Should be Set() once in each
Please replace "apps system" with an appropriate term (e.g. app window system).

https://codereview.chromium.org/583583008/diff/60001/extensions/browser/app_w...
extensions/browser/app_window/app_window_client.h:55: // Return the apps client.
Please replace "apps client" with an appropriate term (e.g. app window client).

https://codereview.chromium.org/583583008/diff/60001/extensions/shell/browser...
File extensions/shell/browser/shell_browser_main_parts.h (right):

https://codereview.chromium.org/583583008/diff/60001/extensions/shell/browser...
extensions/shell/browser/shell_browser_main_parts.h:77:
scoped_ptr<AppWindowClient> apps_client_;
Rename this variable?

[James Cook, 2014-09-19 03:26:07]

https://codereview.chromium.org/583583008/diff/20001/extensions/browser/app_w...
File extensions/browser/app_window/app_window_client.h (right):

https://codereview.chromium.org/583583008/diff/20001/extensions/browser/app_w...
extensions/browser/app_window/app_window_client.h:25: // TODO(hashimoto): Move
some functions to ExtensionsClient.
On 2014/09/19 02:44:37, hashimoto wrote:
> On 2014/09/18 22:29:12, James Cook wrote:
> > On 2014/09/18 21:39:32, oshima wrote:
> > > I'll leave this to hashimoto@ :)
> > 
> > SGTM
> > 
> > GetLoadedBrowserContexts() should move to ExtensionsBrowserClient
> GetLoadedBrowserContexts() is used only by the following static methods of
> AppWindowRegistry.
> -AppWindowRegistry::GetAppWindowForNativeWindowAnyProfile
> -AppWindowRegistry::IsAppWindowRegisteredInAnyProfile
> -AppWindowRegistry::CloseAllAppWindows
> Since all of them are used only by chrome, I began to think these static
methods
> should be split from AppWindowRegistry and put in somewhere (e.g.
> src/apps/app_window_registry_util.{cc,h}) so that we can get rid of
> GetLoadedBrowserContexts() from the client at all.
> Do you think GetLoadedBrowserContexts() can be useful for binaries other than
> chrome (e.g. app_shell and athena)?
> > 
> > IsCurrentChannelOlderThanDev() is not a good API function. I think we need
> > something that takes a channel string, like AllowFeatureFromChannel(const
> > std::string& feature_channel). This should live in ExtensionsClient.
> Is it OK to mention the idea of "channels" in extensions code?
> If it's OK, then maybe we should move
> chrome/common/extensions/features/feature_channel.h to src/extensions?
> > 
> > hashimoto, do you have time to do this?  If you are busy, assign a bug to me
> and
> > I'll do it.
> 

I don't think app_shell or athena will need GetLoadedBrowserContexts().  Moving
those static methods back into chrome sounds good.

Channels are complex. I filed a bug -- let's talk more there.
https://code.google.com/p/chromium/issues/detail?id=415861

[oshima, 2014-09-19 05:23:59]

On 2014/09/19 02:44:38, hashimoto wrote:
> lgtm
> Please also update variables' names and comments.
> 
>
https://codereview.chromium.org/583583008/diff/20001/extensions/browser/app_w...
> File extensions/browser/app_window/app_window_client.h (right):
> 
>
https://codereview.chromium.org/583583008/diff/20001/extensions/browser/app_w...
> extensions/browser/app_window/app_window_client.h:25: // TODO(hashimoto): Move
> some functions to ExtensionsClient.
> On 2014/09/18 22:29:12, James Cook wrote:
> > On 2014/09/18 21:39:32, oshima wrote:
> > > I'll leave this to hashimoto@ :)
> > 
> > SGTM
> > 
> > GetLoadedBrowserContexts() should move to ExtensionsBrowserClient
> GetLoadedBrowserContexts() is used only by the following static methods of
> AppWindowRegistry.
> -AppWindowRegistry::GetAppWindowForNativeWindowAnyProfile
> -AppWindowRegistry::IsAppWindowRegisteredInAnyProfile
> -AppWindowRegistry::CloseAllAppWindows
> Since all of them are used only by chrome, I began to think these static
methods
> should be split from AppWindowRegistry and put in somewhere (e.g.
> src/apps/app_window_registry_util.{cc,h}) so that we can get rid of
> GetLoadedBrowserContexts() from the client at all.
> Do you think GetLoadedBrowserContexts() can be useful for binaries other than
> chrome (e.g. app_shell and athena)?
> > 
> > IsCurrentChannelOlderThanDev() is not a good API function. I think we need
> > something that takes a channel string, like AllowFeatureFromChannel(const
> > std::string& feature_channel). This should live in ExtensionsClient.
> Is it OK to mention the idea of "channels" in extensions code?
> If it's OK, then maybe we should move
> chrome/common/extensions/features/feature_channel.h to src/extensions?
> > 
> > hashimoto, do you have time to do this?  If you are busy, assign a bug to me
> and
> > I'll do it.
> 
>
https://codereview.chromium.org/583583008/diff/60001/athena/extensions/chrome...
> File athena/extensions/chrome/extensions_delegate_impl.cc (right):
> 
>
https://codereview.chromium.org/583583008/diff/60001/athena/extensions/chrome...
> athena/extensions/chrome/extensions_delegate_impl.cc:111:
> AthenaChromeAppWindowClient apps_client_;
> Also rename this variable?
> 
>
https://codereview.chromium.org/583583008/diff/60001/athena/extensions/shell/...
> File athena/extensions/shell/extensions_delegate_impl.cc (right):
> 
>
https://codereview.chromium.org/583583008/diff/60001/athena/extensions/shell/...
> athena/extensions/shell/extensions_delegate_impl.cc:50: 
> Also rename this variable?
> 
>
https://codereview.chromium.org/583583008/diff/60001/extensions/browser/app_w...
> File extensions/browser/app_window/app_window.cc (right):
> 
>
https://codereview.chromium.org/583583008/diff/60001/extensions/browser/app_w...
> extensions/browser/app_window/app_window.cc:275: AppWindowClient* apps_client
=
> AppWindowClient::Get();
> Rename this variable?
> 
>
https://codereview.chromium.org/583583008/diff/60001/extensions/browser/app_w...
> File extensions/browser/app_window/app_window_client.h (right):
> 
>
https://codereview.chromium.org/583583008/diff/60001/extensions/browser/app_w...
> extensions/browser/app_window/app_window_client.h:23: // Sets up global state
> for the apps system. Should be Set() once in each
> Please replace "apps system" with an appropriate term (e.g. app window
system).
> 
>
https://codereview.chromium.org/583583008/diff/60001/extensions/browser/app_w...
> extensions/browser/app_window/app_window_client.h:55: // Return the apps
client.
> Please replace "apps client" with an appropriate term (e.g. app window
client).
> 
>
https://codereview.chromium.org/583583008/diff/60001/extensions/shell/browser...
> File extensions/shell/browser/shell_browser_main_parts.h (right):
> 
>
https://codereview.chromium.org/583583008/diff/60001/extensions/shell/browser...
> extensions/shell/browser/shell_browser_main_parts.h:77:
> scoped_ptr<AppWindowClient> apps_client_;
> Rename this variable?

all done.

[oshima, 2014-09-19 05:28:52]

The CQ bit was checked by oshima@chromium.org

[commit-bot: I haz the power, 2014-09-19 05:29:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/583583008/120001

[hashimoto, 2014-09-19 05:31:14]

lgtm

[commit-bot: I haz the power, 2014-09-19 06:35:13]

Committed patchset #6 (id:120001) as f42529034ed4c99b759d5af218952ac23cdc64a8

[commit-bot: I haz the power, 2014-09-19 06:36:00]

Patchset 6 (id:??) landed as
https://crrev.com/8304b516b8d4463493fd0055a7f2e03e2d182838
Cr-Commit-Position: refs/heads/master@{#295671}