MD Settings: enhance restarting Chrome + interacting w/ session restore

MD Settings: enhance restarting Chrome + interacting w/ session restore

When a user has a tab with chrome://settings open and they restart, and
that restart causes the MaterialDesignSettings feature to be enabled or
disabled, we should do sane things.  Currently, this means nuking
session restore data for those URLs.  This is how history works.

BUG=700350
R=sky@chromium.org

[Dan Beam, 2017-03-14 06:38:35]

The CQ bit was checked by dbeam@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-14 06:38:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-14 06:42:17]

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

[commit-bot: I haz the power, 2017-03-14 06:42:18]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[sky, 2017-03-14 18:02:03]

LGTM

[Dan Beam, 2017-03-17 01:46:16]

dbeam@chromium.org changed reviewers:
+ creis@chromium.org

[Dan Beam, 2017-03-17 01:46:17]

+creis@ for content/OWNERS (moved some URL constants from
chrome/common/url_constants.h to content/public/common/url_constants.h)

[Dan Beam, 2017-03-17 01:46:21]

The CQ bit was checked by dbeam@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-17 01:46:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-17 03:43:15]

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

[commit-bot: I haz the power, 2017-03-17 03:43:16]

Dry run: This issue passed the CQ dry run.

[Dan Beam, 2017-03-17 05:44:17]

Description was changed from

==========
MD Settings: enhance restarting Chrome + interacting w/ session restore

When a user has a tab with chrome://settings open and they restart, and
that restart causes the MaterialDesignSettings feature to be enabled or
disabled, we should do sane things.  Currently, this means nuking
session restore data for those URLs.  This is how history works.

BUG=425627
R=sky@chromium.org
==========

to

==========
MD Settings: enhance restarting Chrome + interacting w/ session restore

When a user has a tab with chrome://settings open and they restart, and
that restart causes the MaterialDesignSettings feature to be enabled or
disabled, we should do sane things.  Currently, this means nuking
session restore data for those URLs.  This is how history works.

BUG=700350
R=sky@chromium.org
==========

[Charlie Reis, 2017-03-17 17:43:11]

creis@chromium.org changed reviewers:
+ jam@chromium.org

[Charlie Reis, 2017-03-17 17:43:12]

Fix looks good.  One concern about the content API, which jam@ might be able to
help with.

https://codereview.chromium.org/2743323005/diff/20001/content/public/common/u...
File content/public/common/url_constants.h (right):

https://codereview.chromium.org/2743323005/diff/20001/content/public/common/u...
content/public/common/url_constants.h:39: CONTENT_EXPORT extern const char
kChromeUIHelpHost[];
[+jam, nasko, twellington, calamity]

Hmm, these aren't used within content/, so I don't think they belong here. 
There's a rule about classes, structs, and enums in the API guidelines, which I
assume applies to constants as well:

https://www.chromium.org/developers/content-module/content-api
"all classes/structs/enums in the public API must be used by embedders and
content. i.e. if the chrome layer uses a struct but content doesn't know about
it, it doesn't belong in content/public but instead some module that's higher
level."

I see that this was already done for things like kChromeUIHistoryURL in
https://codereview.chromium.org/2676893002 and kChromeUIUberHost in
https://codereview.chromium.org/2355543003 (where I apparently missed it).

jam@: Is this acceptable for constants?  Or is there another way for chrome/ and
components/ to share a constant like this?

[sky, 2017-03-17 19:11:41]

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

[sky, 2017-03-17 19:11:42]

In looking at this again I don't think we should be adding constants to content
that are only used in chrome. Not LGTM

https://codereview.chromium.org/2743323005/diff/20001/content/public/common/u...
File content/public/common/url_constants.h (right):

https://codereview.chromium.org/2743323005/diff/20001/content/public/common/u...
content/public/common/url_constants.h:39: CONTENT_EXPORT extern const char
kChromeUIHelpHost[];
On 2017/03/17 17:43:12, Charlie Reis (slow) wrote:
> [+jam, nasko, twellington, calamity]
> 
> Hmm, these aren't used within content/, so I don't think they belong here. 
> There's a rule about classes, structs, and enums in the API guidelines, which
I
> assume applies to constants as well:
> 
> https://www.chromium.org/developers/content-module/content-api
> "all classes/structs/enums in the public API must be used by embedders and
> content. i.e. if the chrome layer uses a struct but content doesn't know about
> it, it doesn't belong in content/public but instead some module that's higher
> level."
> 
> I see that this was already done for things like kChromeUIHistoryURL in
> https://codereview.chromium.org/2676893002 and kChromeUIUberHost in
> https://codereview.chromium.org/2355543003 (where I apparently missed it).
> 
> jam@: Is this acceptable for constants?  Or is there another way for chrome/
and
> components/ to share a constant like this?

I suspect these constants were added here at least partially for
session_restore. I don't think we should do that. Instead session restore should
callout to the driver so that these constants can be defined in chrome. See
701605 for details on what I mean.

[Dan Beam, 2017-03-17 19:54:07]

sorry, forgot to publish

https://codereview.chromium.org/2743323005/diff/20001/content/public/common/u...
File content/public/common/url_constants.h (right):

https://codereview.chromium.org/2743323005/diff/20001/content/public/common/u...
content/public/common/url_constants.h:39: CONTENT_EXPORT extern const char
kChromeUIHelpHost[];
On 2017/03/17 17:43:12, Charlie Reis (slow) wrote:
> [+jam, nasko, twellington, calamity]
> 
> Hmm, these aren't used within content/, so I don't think they belong here. 
> There's a rule about classes, structs, and enums in the API guidelines, which
I
> assume applies to constants as well:
> 
> https://www.chromium.org/developers/content-module/content-api
> "all classes/structs/enums in the public API must be used by embedders and
> content. i.e. if the chrome layer uses a struct but content doesn't know about
> it, it doesn't belong in content/public but instead some module that's higher
> level."
> 
> I see that this was already done for things like kChromeUIHistoryURL in
> https://codereview.chromium.org/2676893002 and kChromeUIUberHost in
> https://codereview.chromium.org/2355543003 (where I apparently missed it).
> 
> jam@: Is this acceptable for constants?  Or is there another way for chrome/
and
> components/ to share a constant like this?

FWIW: thestig@ noticed this while I was de-duping these constants as well:
https://codereview.chromium.org/2751653004/
https://bugs.chromium.org/p/chromium/issues/detail?id=701605

I just don't know of any alternatives currently.  Maybe something with
ContentClient?

And yes, I am basically following the same way we fixed session restore for
history:
https://codereview.chromium.org/2355543003

I guess you're now saying you wished that had been done differently?

All webui has a deep, hard dependency on content/ because that's where all of
the machinery for webui lives.  So trying to say "the history page is
chrome-specific" is true, but all the guts still live in content/.  I get that
other embedders use the content/ webui code as well, but do they use the
chrome:// protocol or other kChromeUI* constants?

By the way, I assume you probably saw, but we can't ship the new Material Design
settings page without this CL to fix the session restore behavior.  I'm not
saying the code needs to look like this, but it's a top priority for me to
unblock this.

When I think of code complexity problems in Chrome I think of layering gray
areas like this :(.

[sky, 2017-03-17 20:02:15]

I'm not a content owner and you have John on this review. I'll let him
comment on whether we should continue down this path.

  -Scott

On Fri, Mar 17, 2017 at 12:54 PM,  <dbeam@chromium.org> wrote:
> sorry, forgot to publish
>
>
>
>
https://codereview.chromium.org/2743323005/diff/20001/content/public/common/u...
> File content/public/common/url_constants.h (right):
>
>
https://codereview.chromium.org/2743323005/diff/20001/content/public/common/u...
> content/public/common/url_constants.h:39: CONTENT_EXPORT extern const
> char kChromeUIHelpHost[];
> On 2017/03/17 17:43:12, Charlie Reis (slow) wrote:
>> [+jam, nasko, twellington, calamity]
>>
>> Hmm, these aren't used within content/, so I don't think they belong
> here.
>> There's a rule about classes, structs, and enums in the API
> guidelines, which I
>> assume applies to constants as well:
>>
>> https://www.chromium.org/developers/content-module/content-api
>> "all classes/structs/enums in the public API must be used by embedders
> and
>> content. i.e. if the chrome layer uses a struct but content doesn't
> know about
>> it, it doesn't belong in content/public but instead some module that's
> higher
>> level."
>>
>> I see that this was already done for things like kChromeUIHistoryURL
> in
>> https://codereview.chromium.org/2676893002 and kChromeUIUberHost in
>> https://codereview.chromium.org/2355543003 (where I apparently missed
> it).
>>
>> jam@: Is this acceptable for constants? Or is there another way for
> chrome/ and
>> components/ to share a constant like this?
>
> FWIW: thestig@ noticed this while I was de-duping these constants as
> well:
> https://codereview.chromium.org/2751653004/
> https://bugs.chromium.org/p/chromium/issues/detail?id=701605
>
> I just don't know of any alternatives currently. Maybe something with
> ContentClient?
>
> And yes, I am basically following the same way we fixed session restore
> for history:
> https://codereview.chromium.org/2355543003
>
> I guess you're now saying you wished that had been done differently?
>
> All webui has a deep, hard dependency on content/ because that's where
> all of the machinery for webui lives. So trying to say "the history
> page is chrome-specific" is true, but all the guts still live in
> content/. I get that other embedders use the content/ webui code as
> well, but do they use the chrome:// protocol or other kChromeUI*
> constants?
>
> By the way, I assume you probably saw, but we can't ship the new
> Material Design settings page without this CL to fix the session restore
> behavior. I'm not saying the code needs to look like this, but it's a
> top priority for me to unblock this.
>
> When I think of code complexity problems in Chrome I think of layering
> gray areas like this :(.
>
> https://codereview.chromium.org/2743323005/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[jam, 2017-03-17 21:05:46]

kChromeUIHelpHost, like kChromeUIHistoryHost, are for chrome features and so
don't belong in content. The previous change, and this one, moved it to content
just because it's a convenient place that chrome and the component can depend
on, but that doesn't change the fact that content doesn't know about schemes
that embedders like chrome might use.
https://www.chromium.org/developers/content-module

The solution here would be to move history and help back to chrome, and chrome
can pass these on to content_serialized_navigation_driver through some
configuration method at startup (i.e. 
ContentSerializedNavigationDriver:ConfigureChromeURLS(GURL history, GURL help).
we should also undo the chrome specific schemes that got added in
https://codereview.chromium.org/2676893002 and 
https://codereview.chromium.org/2355543003. This shouldn't be much work.

[Theresa, 2017-03-17 21:32:14]

On 2017/03/17 21:05:46, jam wrote:
> kChromeUIHelpHost, like kChromeUIHistoryHost, are for chrome features and so
> don't belong in content. The previous change, and this one, moved it to
content
> just because it's a convenient place that chrome and the component can depend
> on, but that doesn't change the fact that content doesn't know about schemes
> that embedders like chrome might use.
> https://www.chromium.org/developers/content-module
> 
> The solution here would be to move history and help back to chrome, and chrome
> can pass these on to content_serialized_navigation_driver through some
> configuration method at startup (i.e. 
> ContentSerializedNavigationDriver:ConfigureChromeURLS(GURL history, GURL
help).
> we should also undo the chrome specific schemes that got added in
> https://codereview.chromium.org/2676893002 and 
> https://codereview.chromium.org/2355543003. This shouldn't be much work.

If we're going that route, we should also pass in the strings for the Chrome for
Android NTP redirect that were hard-coded in
content_serialization_navigation.driver.cc a couple of years ago.