[Android] Rewrite old-style NTP URLs to new-style URLs.

[Android] Rewrite old-style NTP URLs to new-style URLs.

For example, chrome://newtab#bookmarks becomes
chrome-native://bookmarks. This helps users transition smoothly to the
new NTP: any tabs open to the old NTP when the user updates will start
showing the new NTP. Also: opening a foreign session tab from other
platforms and pressing back until arriving at chrome://newtab will now
show the new NTP.

BUG=354063
NOTRY=true

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

[newt (away), 2014-03-21 17:16:04]

jam/samarth: could you help me understand if I'm using
BrowserURLHandler::AddHandlerPair() correctly here?

My goal is to transparently "redirect" chrome://newtab to
chrome-native://newtab, which is the URL that loads the NTP on Android. I would
like for both the renderer and the NavigationController to report the page's URL
as chrome-native://newtab.

Questions:
 1. When is a reverse handler used, and do I need one?
 2. Is it OK to redirect between unrelated URL schemes (e.g. chrome and
chrome-native), or could that cause problems (e.g. incorrect bindings)?

Thanks!

[jam, 2014-03-21 19:47:26]

lgtm

https://codereview.chromium.org/205983005/diff/1/chrome/browser/android/new_t...
File chrome/browser/android/new_tab_page_url_handler.cc (right):

https://codereview.chromium.org/205983005/diff/1/chrome/browser/android/new_t...
chrome/browser/android/new_tab_page_url_handler.cc:23: if (StartsWithASCII(ref,
"bookmarks", true)) {
nit: there are no constants for these already?

i'm surprised if there aren't. if so, please at least add constants at the top
of this file

[newt (away), 2014-03-21 22:23:26]

On 2014/03/21 19:47:26, jam wrote:
> lgtm
> 
>
https://codereview.chromium.org/205983005/diff/1/chrome/browser/android/new_t...
> File chrome/browser/android/new_tab_page_url_handler.cc (right):
> 
>
https://codereview.chromium.org/205983005/diff/1/chrome/browser/android/new_t...
> chrome/browser/android/new_tab_page_url_handler.cc:23: if
(StartsWithASCII(ref,
> "bookmarks", true)) {
> nit: there are no constants for these already?
> 
> i'm surprised if there aren't. if so, please at least add constants at the top
> of this file

Thanks. Could you also comment on the questions in my first comment:
https://codereview.chromium.org/205983005/#msg1 ?

[samarth, 2014-03-21 22:31:59]

On 2014/03/21 17:16:04, newt wrote:
> jam/samarth: could you help me understand if I'm using
> BrowserURLHandler::AddHandlerPair() correctly here?
> 
> My goal is to transparently "redirect" chrome://newtab to
> chrome-native://newtab, which is the URL that loads the NTP on Android. I
would
> like for both the renderer and the NavigationController to report the page's
URL
> as chrome-native://newtab.
> 
> Questions:
>  1. When is a reverse handler used, and do I need one?
>  2. Is it OK to redirect between unrelated URL schemes (e.g. chrome and
> chrome-native), or could that cause problems (e.g. incorrect bindings)?
> 
> Thanks!

I found the reverse handler to be very confusing. The best way to understand it
is to look at the patch that added it:
http://src.chromium.org/viewvc/chrome?revision=84464&view=revision

[samarth, 2014-03-21 22:32:08]

https://codereview.chromium.org/205983005/diff/1/chrome/browser/android/new_t...
File chrome/browser/android/new_tab_page_url_handler.cc (right):

https://codereview.chromium.org/205983005/diff/1/chrome/browser/android/new_t...
chrome/browser/android/new_tab_page_url_handler.cc:17: // static
It's not really "static".  I'd just leave this out to be not confusing.

[newt (away), 2014-03-26 17:47:25]

https://codereview.chromium.org/205983005/diff/1/chrome/browser/android/new_t...
File chrome/browser/android/new_tab_page_url_handler.cc (right):

https://codereview.chromium.org/205983005/diff/1/chrome/browser/android/new_t...
chrome/browser/android/new_tab_page_url_handler.cc:17: // static
On 2014/03/21 22:32:09, samarth wrote:
> It's not really "static".  I'd just leave this out to be not confusing.

Done.

https://codereview.chromium.org/205983005/diff/1/chrome/browser/android/new_t...
chrome/browser/android/new_tab_page_url_handler.cc:23: if (StartsWithASCII(ref,
"bookmarks", true)) {
On 2014/03/21 19:47:26, jam wrote:
> nit: there are no constants for these already?
> 
> i'm surprised if there aren't. if so, please at least add constants at the top
> of this file

These constants exist only in Java and JavaScript code, and they're going away
soon anyway. Added constants in this file.

[newt (away), 2014-03-26 17:47:34]

The CQ bit was checked by newt@chromium.org

[commit-bot: I haz the power, 2014-03-26 17:48:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/newt@chromium.org/205983005/20001

[newt (away), 2014-03-26 18:13:18]

The CQ bit was unchecked by newt@chromium.org

[newt (away), 2014-03-26 18:16:55]

I've modified chrome_content_client.cc to allow chrome-native URLs to be
correctly parsed.

PTAL.

[newt (away), 2014-03-27 18:12:58]

jam: ping on the small additional change in chrome_content_client.cc

[samarth, 2014-03-27 18:18:27]

On 2014/03/21 22:31:59, samarth wrote:
> On 2014/03/21 17:16:04, newt wrote:
> > jam/samarth: could you help me understand if I'm using
> > BrowserURLHandler::AddHandlerPair() correctly here?
> > 
> > My goal is to transparently "redirect" chrome://newtab to
> > chrome-native://newtab, which is the URL that loads the NTP on Android. I
> would
> > like for both the renderer and the NavigationController to report the page's
> URL
> > as chrome-native://newtab.
> > 
> > Questions:
> >  1. When is a reverse handler used, and do I need one?
> >  2. Is it OK to redirect between unrelated URL schemes (e.g. chrome and
> > chrome-native), or could that cause problems (e.g. incorrect bindings)?
> > 
> > Thanks!
> 
> I found the reverse handler to be very confusing. The best way to understand
it
> is to look at the patch that added it:
> http://src.chromium.org/viewvc/chrome?revision=84464&view=revision

Given, do you think you need a reverse handler?

[newt (away), 2014-03-27 18:34:20]

> Given, do you think you need a reverse handler?

The benefit I see would be that users could open a foreign session from the
phone on desktop, and navigate back to the new tab page. Desktop would see the
URL as chrome://newtab (instead of chrome-native://newtab) and the NTP would
actually be shown.

The complication is that there isn't a one-to-one mapping from chrome-native to
chrome URLs: there's no "recent tabs" page on desktop, and the bookmarks folder
URLs don't easily correspond.

I could map chrome-native://newtab -> chrome://newtab at least.
chrome-native://bookmarks -> chrome://bookmarks might also work, though I'm not
sure how to handle chrome-native://bookmarks/#21, which doesn't correspond to
chrome://bookmarks/#21 . Is it a problem if rewriting and reverse rewriting are
not inverse operations?

[jam, 2014-03-27 22:54:18]

On 2014/03/27 18:34:20, newt wrote:
> > Given, do you think you need a reverse handler?
> 
> The benefit I see would be that users could open a foreign session from the
> phone on desktop, and navigate back to the new tab page. Desktop would see the
> URL as chrome://newtab (instead of chrome-native://newtab) and the NTP would
> actually be shown.
> 
> The complication is that there isn't a one-to-one mapping from chrome-native
to
> chrome URLs: there's no "recent tabs" page on desktop, and the bookmarks
folder
> URLs don't easily correspond.
> 
> I could map chrome-native://newtab -> chrome://newtab at least.
> chrome-native://bookmarks -> chrome://bookmarks might also work, though I'm
not
> sure how to handle chrome-native://bookmarks/#21, which doesn't correspond to
> chrome://bookmarks/#21 . Is it a problem if rewriting and reverse rewriting
are
> not inverse operations?

that seems like an edge case that we don't really need to address?

https://codereview.chromium.org/205983005/diff/40001/chrome/common/chrome_con...
File chrome/common/chrome_content_client.cc (right):

https://codereview.chromium.org/205983005/diff/40001/chrome/common/chrome_con...
chrome/common/chrome_content_client.cc:483:
savable_schemes->push_back(chrome::kChromeNativeScheme);
is this really savable?

[newt (away), 2014-03-28 17:54:44]

https://codereview.chromium.org/205983005/diff/40001/chrome/common/chrome_con...
File chrome/common/chrome_content_client.cc (right):

https://codereview.chromium.org/205983005/diff/40001/chrome/common/chrome_con...
chrome/common/chrome_content_client.cc:483:
savable_schemes->push_back(chrome::kChromeNativeScheme);
On 2014/03/27 22:54:19, jam wrote:
> is this really savable?

Nope.

[jam, 2014-03-28 18:00:24]

lgtm

[samarth, 2014-03-28 18:41:12]

lgtm

[newt (away), 2014-03-28 18:42:39]

The CQ bit was checked by newt@chromium.org

[commit-bot: I haz the power, 2014-03-28 18:45:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/newt@chromium.org/205983005/60001

[commit-bot: I haz the power, 2014-03-28 18:54:13]

Change committed as 260218