Implement settings in a widnow

Implement settings om a window

Adds SettingsWindowManager which manages settings popup windows, one per Profile.

Implemented behind the flag -enable-settings-window

Includes some cleanup in chrome_pages for settings url generation and showing settings by profile in general.
Also moves logic for whether to display the Location Bar in a Browser to ShowLocationBar() (affects code for streamlined hosted apps, shouldn't change the logic any).

BUG=342983
R=ben@chromium.org, calamity@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=258615

[stevenjb, 2014-03-03 17:27:41]

Ping

[Ben Goodger (Google), 2014-03-03 18:55:15]

https://codereview.chromium.org/178863002/diff/80001/chrome/browser/ui/browse...
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/178863002/diff/80001/chrome/browser/ui/browse...
chrome/browser/ui/browser.cc:2169: bool Browser::ShowLocationBar() const {
Here's a question... can settings shown in a popup use APP_TYPE_HOST  for its
Browser? APP_TYPE_HOST windows shouldn't get a location bar, so you wouldn't
need to special case the settings page in this function.

https://codereview.chromium.org/178863002/diff/80001/chrome/browser/ui/browser.h
File chrome/browser/ui/browser.h (right):

https://codereview.chromium.org/178863002/diff/80001/chrome/browser/ui/browse...
chrome/browser/ui/browser.h:776: bool ShowLocationBar() const;
Should probably be ShouldShowLocationBar or CanShowLocationBar. As it stands
this looks more imperative.

[stevenjb, 2014-03-03 23:45:24]

https://codereview.chromium.org/178863002/diff/80001/chrome/browser/ui/browse...
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/178863002/diff/80001/chrome/browser/ui/browse...
chrome/browser/ui/browser.cc:2169: bool Browser::ShowLocationBar() const {
On 2014/03/03 18:55:16, Ben Goodger (Google) wrote:
> Here's a question... can settings shown in a popup use APP_TYPE_HOST  for its
> Browser? APP_TYPE_HOST windows shouldn't get a location bar, so you wouldn't
> need to special case the settings page in this function.

I am reluctant to use APP_TYPE_HOST for something that isn't actually an app,
however I believe this may be an opportunity to eliminate AppType (which I think
is only used for window decoration) and replace it with a more descriptive enum
(or maybe a WindowFeature mask)?

I will put that up as a separate CL for review/discussion.

[Ben Goodger (Google), 2014-03-04 16:36:28]

On 2014/03/03 23:45:24, stevenjb wrote:
> I am reluctant to use APP_TYPE_HOST for something that isn't actually an app,
> however I believe this may be an opportunity to eliminate AppType (which I
think
> is only used for window decoration) and replace it with a more descriptive
enum
> (or maybe a WindowFeature mask)?
> 
> I will put that up as a separate CL for review/discussion.

The enum names have changed since I was last in this code a lot but my sense is
that APP_TYPE_HOST is the original "app frame" code (e.g. Create Application
Shortcuts) which seems appropriate for this change given your intended UI. I'm
all for making that code more general purpose or choosing better names, but as
it stands this CL isn't a step in that direction. Maybe do some history analysis
and figure out who came up with the name APP_TYPE_HOST and CC them too?

[stevenjb, 2014-03-04 17:02:19]

I think that was me :)

The enum is used to differentiate between a Browser associated with a
hosted (v1) app, and a "child" window spawned by an app (e.g. via
window.open). The only places we currently test app_type() is whether or
not to show the url bar and whether to show the title and icon in the frame
in Views.

I did a pass at merging the enum with Type (so that we have TABBED, POPUP,
and HOSTED), but it turned into a pretty large CL, and I'm not entirely
certain it's an improvement. I did find some places where it isn't clear
whether we are setting AppType correctly, so I may at least try to make
that more robust in a separate CL.

I've been trying to come up with better names for the enum instead (TRUSTED
vs UNTRUSTED?, FRAME_NORMAL vs FRAME_APP?) but am not sure I can come with
anything much better, so I may just add a comment to the enum and use that
as suggested.



On Tue, Mar 4, 2014 at 8:36 AM, <ben@chromium.org> wrote:

> On 2014/03/03 23:45:24, stevenjb wrote:
>
>> I am reluctant to use APP_TYPE_HOST for something that isn't actually an
>> app,
>> however I believe this may be an opportunity to eliminate AppType (which I
>>
> think
>
>> is only used for window decoration) and replace it with a more descriptive
>>
> enum
>
>> (or maybe a WindowFeature mask)?
>>
>
>  I will put that up as a separate CL for review/discussion.
>>
>
> The enum names have changed since I was last in this code a lot but my
> sense is
> that APP_TYPE_HOST is the original "app frame" code (e.g. Create
> Application
> Shortcuts) which seems appropriate for this change given your intended UI.
> I'm
> all for making that code more general purpose or choosing better names,
> but as
> it stands this CL isn't a step in that direction. Maybe do some history
> analysis
> and figure out who came up with the name APP_TYPE_HOST and CC them too?
>
> https://codereview.chromium.org/178863002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[calamity, 2014-03-05 02:38:11]

Streamlined hosted apps stuff lgtm.

https://codereview.chromium.org/178863002/diff/80001/chrome/browser/ui/browse...
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/178863002/diff/80001/chrome/browser/ui/browse...
chrome/browser/ui/browser.cc:2200: return false;  // Assume app is non-legacy.
Non-legacy apps would get a location bar. I don't mind whether you change the
value or comment.

[stevenjb, 2014-03-06 23:51:56]

PTAL

https://codereview.chromium.org/178863002/diff/80001/chrome/browser/ui/browse...
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/178863002/diff/80001/chrome/browser/ui/browse...
chrome/browser/ui/browser.cc:2169: bool Browser::ShowLocationBar() const {
On 2014/03/03 23:45:24, stevenjb wrote:
> On 2014/03/03 18:55:16, Ben Goodger (Google) wrote:
> > Here's a question... can settings shown in a popup use APP_TYPE_HOST  for
its
> > Browser? APP_TYPE_HOST windows shouldn't get a location bar, so you wouldn't
> > need to special case the settings page in this function.
> 
> I am reluctant to use APP_TYPE_HOST for something that isn't actually an app,
> however I believe this may be an opportunity to eliminate AppType (which I
think
> is only used for window decoration) and replace it with a more descriptive
enum
> (or maybe a WindowFeature mask)?
> 
> I will put that up as a separate CL for review/discussion.
> 
OK, so I looked through the code and did some experiments. 
APP_TYPE_HOST is the *default* setting. In order to make that meaningful for
non-app windows, we would have to change that i.e. currently if a non app
Browser tab opens a popup window, AppType is APP_TYPE_HOST, which is fine since
it is ignored for non apps). Fixing this turns out to be a fairly invasive and
not entirely straightforward change. 

Instead, I moved the logic below into a helper function in chrome_pages, with
TODO comments to change/simplify it if/when we clean up AppType.

https://codereview.chromium.org/178863002/diff/80001/chrome/browser/ui/browse...
chrome/browser/ui/browser.cc:2196: // legacy packaged apps.
Actually, this comment is wrong. We want to -show- the location bar for
non-legacy packaged apps. Fixed.

https://codereview.chromium.org/178863002/diff/80001/chrome/browser/ui/browse...
chrome/browser/ui/browser.cc:2200: return false;  // Assume app is non-legacy.
On 2014/03/05 02:38:11, calamity wrote:
> Non-legacy apps would get a location bar. I don't mind whether you change the
> value or comment.
Restored the existing logic.

https://codereview.chromium.org/178863002/diff/80001/chrome/browser/ui/browser.h
File chrome/browser/ui/browser.h (right):

https://codereview.chromium.org/178863002/diff/80001/chrome/browser/ui/browse...
chrome/browser/ui/browser.h:776: bool ShowLocationBar() const;
On 2014/03/03 18:55:16, Ben Goodger (Google) wrote:
> Should probably be ShouldShowLocationBar or CanShowLocationBar. As it stands
> this looks more imperative.

Done.

[stevenjb, 2014-03-11 00:43:18]

ping

[stevenjb, 2014-03-14 00:42:23]

Friendly ping. I'm getting some pressure from folks wanting to start playing
with this, but I also want to find something that we are all reasonably
comfortable with, given the nature of the change. Happy to send this someone
else's way though if you can recommend someone. Thanks!

[Ben Goodger (Google), 2014-03-19 17:09:31]

I wish you would have come up with a way to treat this in the same way as app
host, because honestly I don't think there's that much risk/complexity in doing
so, and I'm not sure that there'll be much motivation to do this in the future
("there's no time like the present!")

However this is not my biggest worry today and I want this out of my review
queue, so LGTM and please note my grump above. :-P

[stevenjb, 2014-03-19 17:22:55]

On 2014/03/19 17:09:31, Ben Goodger (Google) wrote:
> I wish you would have come up with a way to treat this in the same way as app
> host, because honestly I don't think there's that much risk/complexity in
doing
> so, and I'm not sure that there'll be much motivation to do this in the future
> ("there's no time like the present!")
> 
> However this is not my biggest worry today and I want this out of my review
> queue, so LGTM and please note my grump above. :-P

Hey Ben, thanks for the review, I know you're swamped.

FWIW, I did spend a fair bit of effort trying, in fact there are two CLs you can
look at if you like, but I wasn't happy enough with either of them to spend more
of your time and mine getting them right. 

* https://codereview.chromium.org/187443008/ (WIP: Elim Browser::AppType)
* https://codereview.chromium.org/187443008/ (AppType -> PopupType)

The underlying problem is that because we default to "APP_TYPE_HOST," which is
the more permissive type, we need to be pretty careful about how we create
Browser instances, and currently we're really not (even though the CreatePrams
is a far cry better than the mismatch of builders we used to have). 

I outlined some of the problems in http://crbug.com/350128.

I promise that if we go forward with this and eliminate the flag, I will address
this issue first.

[stevenjb, 2014-03-19 17:23:12]

The CQ bit was checked by stevenjb@chromium.org

[commit-bot: I haz the power, 2014-03-19 17:24:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevenjb@chromium.org/178863002/120001

[commit-bot: I haz the power, 2014-03-19 17:24:16]

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

[commit-bot: I haz the power, 2014-03-19 17:24:17]

Failed to apply patch for chrome/browser/about_flags.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/about_flags.cc
  Hunk #1 FAILED at 1908.
  1 out of 1 hunk FAILED -- saving rejects to file
chrome/browser/about_flags.cc.rej

Patch:       chrome/browser/about_flags.cc
Index: chrome/browser/about_flags.cc
diff --git a/chrome/browser/about_flags.cc b/chrome/browser/about_flags.cc
index
f11a2c4e7db93665bbb9207f5aaa53e620109901..32aa2461ad21cf3b974bac89136fc79a151cbbcd
100644
--- a/chrome/browser/about_flags.cc
+++ b/chrome/browser/about_flags.cc
@@ -1908,6 +1908,14 @@ const Experiment kExperiments[] = {
     SINGLE_VALUE_TYPE(app_list::switches::kEnableExperimentalAppList)
   },
 #endif
+  {
+    "enable-settings-window",
+    IDS_FLAGS_ENABLE_SETTINGS_WINDOW_NAME,
+    IDS_FLAGS_ENABLE_SETTINGS_WINDOW_DESCRIPTION,
+    kOsDesktop,
+    SINGLE_VALUE_TYPE(switches::kEnableSettingsWindow)
+  },
+
 };
 
 const Experiment* experiments = kExperiments;

[stevenjb, 2014-03-19 17:50:55]

The CQ bit was checked by stevenjb@chromium.org

[commit-bot: I haz the power, 2014-03-19 17:51:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevenjb@chromium.org/178863002/140001

[commit-bot: I haz the power, 2014-03-19 18:29:45]

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

[commit-bot: I haz the power, 2014-03-19 18:29:47]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[stevenjb, 2014-03-19 18:38:39]

The CQ bit was checked by stevenjb@chromium.org

[commit-bot: I haz the power, 2014-03-19 18:39:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevenjb@chromium.org/178863002/150001

[commit-bot: I haz the power, 2014-03-19 18:44:26]

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

[commit-bot: I haz the power, 2014-03-19 18:44:27]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_clang_dbg

[stevenjb, 2014-03-20 20:16:56]

The CQ bit was checked by stevenjb@chromium.org

[commit-bot: I haz the power, 2014-03-20 20:17:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevenjb@chromium.org/178863002/170001

[commit-bot: I haz the power, 2014-03-20 22:04:19]

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

[commit-bot: I haz the power, 2014-03-20 22:04:21]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_rel

[stevenjb, 2014-03-21 00:40:20]

The CQ bit was checked by stevenjb@chromium.org

[commit-bot: I haz the power, 2014-03-21 00:41:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevenjb@chromium.org/178863002/170001

[commit-bot: I haz the power, 2014-03-21 01:04:03]

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

[commit-bot: I haz the power, 2014-03-21 01:04:03]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_rel

[stevenjb, 2014-03-21 17:13:56]

Committed patchset #9 manually as r258615 (presubmit successful).