[MD Settings] PROOF OF CONCEPT - DO NOT LAND - URL rewrite

[MD Settings] PROOF OF CONCEPT - DO NOT LAND - URL rewrite

Depending on the enable-md-settings flag, the actual settings page lies
behind chrome://settings or chrome://chrome/settings. This CL ensures
that restored navigation entries are updated properly, preventing
broken navigation.

PROOF OF CONCEPT - DO NOT LAND

BUG=

[groby-ooo-7-16, 2016-07-20 00:40:23]

Description was changed from

==========
Formatting changes.


[MD Settings] PROOF OF CONCEPT - DO NOT LAND - URL rewrite

Depending on the enable-md-settings flag, the actual settings page lies
behind chrome://settings or chrome://chrome/settings. This CL ensures
that restored navigation entries are updated properly, preventing
broken navigation.

PROOF OF CONCEPT - DO NOT LAND

BUG=
==========

to

==========
Formatting changes.


[MD Settings] PROOF OF CONCEPT - DO NOT LAND - URL rewrite

Depending on the enable-md-settings flag, the actual settings page lies
behind chrome://settings or chrome://chrome/settings. This CL ensures
that restored navigation entries are updated properly, preventing
broken navigation.

PROOF OF CONCEPT - DO NOT LAND

BUG=
==========

[groby-ooo-7-16, 2016-07-20 00:40:24]

groby@chromium.org changed reviewers:
+ dbeam@chromium.org, sky@chromium.org

[groby-ooo-7-16, 2016-07-20 00:51:28]

On 2016/07/20 00:40:24, groby wrote:
> mailto:groby@chromium.org changed reviewers:
> + mailto:dbeam@chromium.org, mailto:sky@chromium.org

dbeam: Heads up - this is the rough outline of the change we need for our
virtual URLs.

sky: We need to rewrite serialized navigation entries, depending on a
Chrome-specific flag. (The md-settings change keeps virtual URLs consistent, but
changes the original URL, which causes issues on reloads across flag changes)

DEPS prevent us from looking at the flag directly, so we need to inject a filter
somehow - is there already another place that injects Chrome dependencies into
the sessions component, or do we need to roll our own?

If we have to roll our own, I'd probably add a "SetUrlSanitizer" function on
SerializedNavigationDriver and call that from Chrome. It probably would need to
be called somewhere around ChromeBrowserMainParts::PreBrowserStart so it's set
up by session restore. The callback would presumably be something like
    std::tuple<GURL, GURL> SanitizeURLs(const GURL& original, const GURL&
virtual);

Thoughts?

[groby-ooo-7-16, 2016-07-20 00:51:48]

Description was changed from

==========
Formatting changes.


[MD Settings] PROOF OF CONCEPT - DO NOT LAND - URL rewrite

Depending on the enable-md-settings flag, the actual settings page lies
behind chrome://settings or chrome://chrome/settings. This CL ensures
that restored navigation entries are updated properly, preventing
broken navigation.

PROOF OF CONCEPT - DO NOT LAND

BUG=
==========

to

==========
[MD Settings] PROOF OF CONCEPT - DO NOT LAND - URL rewrite

Depending on the enable-md-settings flag, the actual settings page lies
behind chrome://settings or chrome://chrome/settings. This CL ensures
that restored navigation entries are updated properly, preventing
broken navigation.

PROOF OF CONCEPT - DO NOT LAND

BUG=
==========

[sky, 2016-07-20 15:32:36]

Rather than a global function in content_serialized_navigation_driver.cc how
about definining a delegate interface that ContentSerializedNavigationDriver
calls to for the serialization? startup_browser_creator_impl.cc would set the
object that does the rewriting on ContentSerializedNavigationDriver. This
provides marginally better scoping than what you have now.

[Dan Beam, 2016-07-21 23:36:18]

https://codereview.chromium.org/2166693002/diff/1/components/sessions/content...
File components/sessions/content/content_serialized_navigation_driver.cc
(right):

https://codereview.chromium.org/2166693002/diff/1/components/sessions/content...
components/sessions/content/content_serialized_navigation_driver.cc:118: if
(g_settingsFlagFunc &&
can we plumb this through ContentBrowserClient?

class ContentBrowserClient {
  virtual GURL RewriteURLForSessionRestore(const GURL& url) const = 0;
};

GURL ChromeContentBrowserClient::RewriteURLForSessionRestore(
    const GURL& url) const {
  GURL reversed_url(url);
  HandleWebUIReverse(&reversed_url, nullptr);
  return reversed_url;
}

[groby-ooo-7-16, 2016-07-26 00:23:10]

So, after doing some digging, it was _almost_ easy, until it wasn't. 

Because at some point, we decided to do BrowserContext-dependent URL rewrites.
But kept the session service BrowserContext-agnostic.

https://codereview.chromium.org/2166693002/diff/1/components/sessions/content...
File components/sessions/content/content_serialized_navigation_driver.cc
(right):

https://codereview.chromium.org/2166693002/diff/1/components/sessions/content...
components/sessions/content/content_serialized_navigation_driver.cc:118: if
(g_settingsFlagFunc &&
On 2016/07/21 23:36:18, Dan Beam wrote:
> can we plumb this through ContentBrowserClient?

That was the plan, until I started digging deeper...

> 
> class ContentBrowserClient {
>   virtual GURL RewriteURLForSessionRestore(const GURL& url) const = 0;
> };
> 
> GURL ChromeContentBrowserClient::RewriteURLForSessionRestore(
>     const GURL& url) const {
>   GURL reversed_url(url);
>   HandleWebUIReverse(&reversed_url, nullptr);
>   return reversed_url;
> }

Not quite as trivial, since we also need to add the uber host, depending on what
direction we're going. The function that does that is WillHandleBrowserAboutURL.
 

And if we wanted to do the "right" thing, we essentially need to do
BrowserURLHandlerImpl::RewriteURLIfNecessary, but restricted to the handlers we
care about (i.e. WebUI)

Which we could do without adding additional hooks:

    if (navigation->virtual_url_.SchemeIs(content::kChromeUIScheme)) {
        BrowserURLHandlerImpl::GetInstance()->RewriteURLIfNecessary(
            &navigation->original_request_url_, browser_context,
&reverse_on_redirect);
        navigation->encoded_page_state_ =
            content::PageState::CreateFromURL(navigation->original_request_url_)
                 .ToEncodedData();
    }

Except the missing |browser_context_| here. I'm not sure why we thought that
rewriting URLs based on profile is ever a good idea, but it doesn't look like
one from here. (I'm wildly speculating that extensions that register URL
overrides will also run into trouble if they change their overrides. As will NTP
changes. Oh well.)

Anyways, that means none of the above schemes will work, and things need to be
handcoded. Yay us.

We _could_ hack things so that WillHandleBrowserAboutURL can deal with nullptrs,
but we might as well just hardcode the URL handling as a separate callback at
this point.

So close, and yet so far.

[Dan Beam, 2016-07-26 00:29:16]

On 2016/07/26 00:23:10, groby wrote:
> So, after doing some digging, it was _almost_ easy, until it wasn't. 
> 
> Because at some point, we decided to do BrowserContext-dependent URL rewrites.
> But kept the session service BrowserContext-agnostic.
> 
>
https://codereview.chromium.org/2166693002/diff/1/components/sessions/content...
> File components/sessions/content/content_serialized_navigation_driver.cc
> (right):
> 
>
https://codereview.chromium.org/2166693002/diff/1/components/sessions/content...
> components/sessions/content/content_serialized_navigation_driver.cc:118: if
> (g_settingsFlagFunc &&
> On 2016/07/21 23:36:18, Dan Beam wrote:
> > can we plumb this through ContentBrowserClient?
> 
> That was the plan, until I started digging deeper...
> 
> > 
> > class ContentBrowserClient {
> >   virtual GURL RewriteURLForSessionRestore(const GURL& url) const = 0;
> > };
> > 
> > GURL ChromeContentBrowserClient::RewriteURLForSessionRestore(
> >     const GURL& url) const {
> >   GURL reversed_url(url);
> >   HandleWebUIReverse(&reversed_url, nullptr);
> >   return reversed_url;
> > }
> 
> Not quite as trivial, since we also need to add the uber host, depending on
what
> direction we're going. The function that does that is
WillHandleBrowserAboutURL.
>  
> 
> And if we wanted to do the "right" thing, we essentially need to do
> BrowserURLHandlerImpl::RewriteURLIfNecessary, but restricted to the handlers
we
> care about (i.e. WebUI)
> 
> Which we could do without adding additional hooks:
> 
>     if (navigation->virtual_url_.SchemeIs(content::kChromeUIScheme)) {
>         BrowserURLHandlerImpl::GetInstance()->RewriteURLIfNecessary(
>             &navigation->original_request_url_, browser_context,
> &reverse_on_redirect);
>         navigation->encoded_page_state_ =
>            
content::PageState::CreateFromURL(navigation->original_request_url_)
>                  .ToEncodedData();
>     }
> 
> Except the missing |browser_context_| here. I'm not sure why we thought that
> rewriting URLs based on profile is ever a good idea, but it doesn't look like
> one from here. (I'm wildly speculating that extensions that register URL
> overrides will also run into trouble if they change their overrides. As will
NTP
> changes. Oh well.)
> 
> Anyways, that means none of the above schemes will work, and things need to be
> handcoded. Yay us.
> 
> We _could_ hack things so that WillHandleBrowserAboutURL can deal with
nullptrs,
> but we might as well just hardcode the URL handling as a separate callback at
> this point.
> 
> So close, and yet so far.

the browsercontext used when rewriting is not necessary, can't we just drop it? 
pretty sure it was added only because some callback needed it as a param.

[groby-ooo-7-16, 2016-07-26 00:43:11]

On 2016/07/26 00:29:16, Dan Beam wrote:
> On 2016/07/26 00:23:10, groby wrote:
> > So, after doing some digging, it was _almost_ easy, until it wasn't. 
> > 
> > Because at some point, we decided to do BrowserContext-dependent URL
rewrites.
> > But kept the session service BrowserContext-agnostic.
> > 
> >
>
https://codereview.chromium.org/2166693002/diff/1/components/sessions/content...
> > File components/sessions/content/content_serialized_navigation_driver.cc
> > (right):
> > 
> >
>
https://codereview.chromium.org/2166693002/diff/1/components/sessions/content...
> > components/sessions/content/content_serialized_navigation_driver.cc:118: if
> > (g_settingsFlagFunc &&
> > On 2016/07/21 23:36:18, Dan Beam wrote:
> > > can we plumb this through ContentBrowserClient?
> > 
> > That was the plan, until I started digging deeper...
> > 
> > > 
> > > class ContentBrowserClient {
> > >   virtual GURL RewriteURLForSessionRestore(const GURL& url) const = 0;
> > > };
> > > 
> > > GURL ChromeContentBrowserClient::RewriteURLForSessionRestore(
> > >     const GURL& url) const {
> > >   GURL reversed_url(url);
> > >   HandleWebUIReverse(&reversed_url, nullptr);
> > >   return reversed_url;
> > > }
> > 
> > Not quite as trivial, since we also need to add the uber host, depending on
> what
> > direction we're going. The function that does that is
> WillHandleBrowserAboutURL.
> >  
> > 
> > And if we wanted to do the "right" thing, we essentially need to do
> > BrowserURLHandlerImpl::RewriteURLIfNecessary, but restricted to the handlers
> we
> > care about (i.e. WebUI)
> > 
> > Which we could do without adding additional hooks:
> > 
> >     if (navigation->virtual_url_.SchemeIs(content::kChromeUIScheme)) {
> >         BrowserURLHandlerImpl::GetInstance()->RewriteURLIfNecessary(
> >             &navigation->original_request_url_, browser_context,
> > &reverse_on_redirect);
> >         navigation->encoded_page_state_ =
> >            
> content::PageState::CreateFromURL(navigation->original_request_url_)
> >                  .ToEncodedData();
> >     }
> > 
> > Except the missing |browser_context_| here. I'm not sure why we thought that
> > rewriting URLs based on profile is ever a good idea, but it doesn't look
like
> > one from here. (I'm wildly speculating that extensions that register URL
> > overrides will also run into trouble if they change their overrides. As will
> NTP
> > changes. Oh well.)
> > 
> > Anyways, that means none of the above schemes will work, and things need to
be
> > handcoded. Yay us.
> > 
> > We _could_ hack things so that WillHandleBrowserAboutURL can deal with
> nullptrs,
> > but we might as well just hardcode the URL handling as a separate callback
at
> > this point.
> > 
> > So close, and yet so far.
> 
> the browsercontext used when rewriting is not necessary, can't we just drop
it? 
> pretty sure it was added only because some callback needed it as a param.

It is :( See WillHandleBrowserAboutURL:88
      if (MdHistoryUI::IsEnabled(Profile::FromBrowserContext(browser_context))) 

I can fix that one up, and just pray any new handlers deal with a nullptr.

[Dan Beam, 2016-07-26 00:50:14]

On 2016/07/26 00:43:11, groby wrote:
> On 2016/07/26 00:29:16, Dan Beam wrote:
> > On 2016/07/26 00:23:10, groby wrote:
> > > So, after doing some digging, it was _almost_ easy, until it wasn't. 
> > > 
> > > Because at some point, we decided to do BrowserContext-dependent URL
> rewrites.
> > > But kept the session service BrowserContext-agnostic.
> > > 
> > >
> >
>
https://codereview.chromium.org/2166693002/diff/1/components/sessions/content...
> > > File components/sessions/content/content_serialized_navigation_driver.cc
> > > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2166693002/diff/1/components/sessions/content...
> > > components/sessions/content/content_serialized_navigation_driver.cc:118:
if
> > > (g_settingsFlagFunc &&
> > > On 2016/07/21 23:36:18, Dan Beam wrote:
> > > > can we plumb this through ContentBrowserClient?
> > > 
> > > That was the plan, until I started digging deeper...
> > > 
> > > > 
> > > > class ContentBrowserClient {
> > > >   virtual GURL RewriteURLForSessionRestore(const GURL& url) const = 0;
> > > > };
> > > > 
> > > > GURL ChromeContentBrowserClient::RewriteURLForSessionRestore(
> > > >     const GURL& url) const {
> > > >   GURL reversed_url(url);
> > > >   HandleWebUIReverse(&reversed_url, nullptr);
> > > >   return reversed_url;
> > > > }
> > > 
> > > Not quite as trivial, since we also need to add the uber host, depending
on
> > what
> > > direction we're going. The function that does that is
> > WillHandleBrowserAboutURL.
> > >  
> > > 
> > > And if we wanted to do the "right" thing, we essentially need to do
> > > BrowserURLHandlerImpl::RewriteURLIfNecessary, but restricted to the
handlers
> > we
> > > care about (i.e. WebUI)
> > > 
> > > Which we could do without adding additional hooks:
> > > 
> > >     if (navigation->virtual_url_.SchemeIs(content::kChromeUIScheme)) {
> > >         BrowserURLHandlerImpl::GetInstance()->RewriteURLIfNecessary(
> > >             &navigation->original_request_url_, browser_context,
> > > &reverse_on_redirect);
> > >         navigation->encoded_page_state_ =
> > >            
> > content::PageState::CreateFromURL(navigation->original_request_url_)
> > >                  .ToEncodedData();
> > >     }
> > > 
> > > Except the missing |browser_context_| here. I'm not sure why we thought
that
> > > rewriting URLs based on profile is ever a good idea, but it doesn't look
> like
> > > one from here. (I'm wildly speculating that extensions that register URL
> > > overrides will also run into trouble if they change their overrides. As
will
> > NTP
> > > changes. Oh well.)
> > > 
> > > Anyways, that means none of the above schemes will work, and things need
to
> be
> > > handcoded. Yay us.
> > > 
> > > We _could_ hack things so that WillHandleBrowserAboutURL can deal with
> > nullptrs,
> > > but we might as well just hardcode the URL handling as a separate callback
> at
> > > this point.
> > > 
> > > So close, and yet so far.
> > 
> > the browsercontext used when rewriting is not necessary, can't we just drop
> it? 
> > pretty sure it was added only because some callback needed it as a param.
> 
> It is :( See WillHandleBrowserAboutURL:88
>       if
(MdHistoryUI::IsEnabled(Profile::FromBrowserContext(browser_context))) 
> 
> I can fix that one up, and just pray any new handlers deal with a nullptr.

but MdHistoryUI::IsEnabled() actually requires asking whether
profile->IsSupervised().

could we add a Profile* arg from the Chrome-side implementation of
ContentBrowserClient?

[groby-ooo-7-16, 2016-07-26 02:06:50]

sky: PTAL - thank you!

> could we add a Profile* arg from the Chrome-side implementation of
> ContentBrowserClient?

Alas, no. ContentBrowserClient is profile-agnostic. In fact, the entire
deserialization pipeline is profile agnostic. Given that 
a) we do profile-specific URL rewrites
b) the mappings between virtual and request URL can change between versions

that's a problem. Not one I'm necessarily prepared to tackle here. 

Anyways. Provided a new version that uses existing hooks. It does slightly scary
things, but so does every other approach.

[sky, 2016-07-26 16:22:07]

Seems reasonable to me. How about test coverage?

[groby-ooo-7-16, 2016-07-26 20:07:28]

On 2016/07/26 16:22:07, sky wrote:
> Seems reasonable to me. How about test coverage?

Looking at it. Wrangling with resetting state, because _of course_ this is a
singleton. NEED MOAR SINGLETONS.

[Dan Beam, 2016-10-14 03:57:37]

can we close this now?  calamity@ fixed this for history

[groby-ooo-7-16, 2016-10-14 18:39:23]

On 2016/10/14 03:57:37, Dan Beam wrote:
> can we close this now?  calamity@ fixed this for history

Closed. It was fun while it lasted :)