Rachel's working the most on this right now

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/12483f26c404123844df38d8e8083c1d6dde1426

commit 12483f26c404123844df38d8e8083c1d6dde1426
Author: groby <groby@chromium.org>
Date: Tue Jun 14 02:12:45 2016

[MD Settings] Add feature to enable md-settings by default.

chrome://settings will serve either old settings or md-settings,
depending on flag. chrome://settings-frame will always serve old
settings. chrome://md-settings will serve new settings, unless
the flag is off.

BUG= 614758 

Review-Url: https://codereview.chromium.org/2029263002
Cr-Commit-Position: refs/heads/master@{#399640}

[modify] https://crrev.com/12483f26c404123844df38d8e8083c1d6dde1426/chrome/app/generated_resources.grd
[modify] https://crrev.com/12483f26c404123844df38d8e8083c1d6dde1426/chrome/browser/about_flags.cc
[modify] https://crrev.com/12483f26c404123844df38d8e8083c1d6dde1426/chrome/browser/browser_about_handler.cc
[modify] https://crrev.com/12483f26c404123844df38d8e8083c1d6dde1426/chrome/browser/profiles/profile_window.cc
[modify] https://crrev.com/12483f26c404123844df38d8e8083c1d6dde1426/chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc
[modify] https://crrev.com/12483f26c404123844df38d8e8083c1d6dde1426/chrome/browser/ui/webui/settings/md_settings_ui.cc
[modify] https://crrev.com/12483f26c404123844df38d8e8083c1d6dde1426/chrome/browser/ui/webui/uber/uber_ui.cc
[modify] https://crrev.com/12483f26c404123844df38d8e8083c1d6dde1426/chrome/browser/ui/webui/uber/uber_ui_browsertest.cc
[modify] https://crrev.com/12483f26c404123844df38d8e8083c1d6dde1426/chrome/common/chrome_features.cc
[modify] https://crrev.com/12483f26c404123844df38d8e8083c1d6dde1426/chrome/common/chrome_features.h

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/12483f26c404123844df38d8e8083c1d6dde1426

commit 12483f26c404123844df38d8e8083c1d6dde1426
Author: groby <groby@chromium.org>
Date: Tue Jun 14 02:12:45 2016

[MD Settings] Add feature to enable md-settings by default.

chrome://settings will serve either old settings or md-settings,
depending on flag. chrome://settings-frame will always serve old
settings. chrome://md-settings will serve new settings, unless
the flag is off.

BUG= 614758 

Review-Url: https://codereview.chromium.org/2029263002
Cr-Commit-Position: refs/heads/master@{#399640}

[modify] https://crrev.com/12483f26c404123844df38d8e8083c1d6dde1426/chrome/app/generated_resources.grd
[modify] https://crrev.com/12483f26c404123844df38d8e8083c1d6dde1426/chrome/browser/about_flags.cc
[modify] https://crrev.com/12483f26c404123844df38d8e8083c1d6dde1426/chrome/browser/browser_about_handler.cc
[modify] https://crrev.com/12483f26c404123844df38d8e8083c1d6dde1426/chrome/browser/profiles/profile_window.cc
[modify] https://crrev.com/12483f26c404123844df38d8e8083c1d6dde1426/chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc
[modify] https://crrev.com/12483f26c404123844df38d8e8083c1d6dde1426/chrome/browser/ui/webui/settings/md_settings_ui.cc
[modify] https://crrev.com/12483f26c404123844df38d8e8083c1d6dde1426/chrome/browser/ui/webui/uber/uber_ui.cc
[modify] https://crrev.com/12483f26c404123844df38d8e8083c1d6dde1426/chrome/browser/ui/webui/uber/uber_ui_browsertest.cc
[modify] https://crrev.com/12483f26c404123844df38d8e8083c1d6dde1426/chrome/common/chrome_features.cc
[modify] https://crrev.com/12483f26c404123844df38d8e8083c1d6dde1426/chrome/common/chrome_features.h

 Issue 637620  has been merged into this issue.

this is required to launch chrome://history, and calamity@ may have more spare cycles than groby@ to look at this in the near future, so re-assigning to him.

Chris: here's the proof-of-concept CL
https://codereview.chromium.org/2166693002/

We also talked about "use Material Design" check lower in the code and returning a wrapper WebUI if necessary.  this may be required if session restore doesn't have access to profiles, and we need to know whether the profile is supervised to know whether to "use Material Design" for the history UI or not.

creis@: I see there's a blocking bug here dealing with navigation.  virtual URL wonkiness is blocking the Material Design history page from launching.  what can we do to speed up a fix?  if the finch config changes to enable Material Design history, and the user has chrome://history open, they're greeted by a broken page on next restart :(

Passing over to lshang@ to investigate using the wrapper WebUI.

Bleh. So I took a bit of a look into this and I don't know how a wrapper ui would work here. My take on the situation is this (after applying groby's patch):

- chrome:history gets rewritten to chrome:chrome/history on session restore (in browser_about_handler.cc) because we don't know if the profile is supervised at that time so we take MD History as disabled.
- By the time the WebUI is _actually_ specified (in GetWebUIFactoryFunction), we do have a profile, so the MdHistoryUI is returned.
- We register as a handler of chrome:history (via content::WebUIDataSource::Add), then the URLDataManagerBackend is all like 'gimme the data for chrome:chrome/history', and we proceed to drop everything on the floor.

Without the patch, the same thing happens because there just isn't any URL rewriting.

I think there might also be some virtual vs actual URL funniness going on because during restore, the WebUIControllerFactory is getting told to make WebUIs for both chrome:history and chrome:chrome/history.

But, weirdly, my machine just started properly restoring chrome:chrome/history halfway through my investigation. I think there's a race acting funny... I'll try to get back to the broken state tomorrow.

Removing the IsMdHistoryEnabled() check from the WebUIControllerFactory is enough to get refresh working.

We are have a scheduled Beta Release today. Can anyone confirm whether this is a Beta blocker? OR can it wait until next weeks release?

This is the only release blocker we have as of now. Please confirm ASAP.

c#19 - I'm not sure if this
Thanks for checking this out, calamity@

Quick Q: Why does a supervised profile manage to URL rewriting? Didn't https://codereview.chromium.org/1993613002 implement the necessary UI?

> I think there might also be some virtual vs actual URL funniness going on 
No kidding. I _think_ it's related to the fact that SessionState also contains URLs (you can never have enough), but I haven't managed to completely grok what's going on there. The main point of the patch was to wipe SesssionState - IIRC, it'd work with keeping the URLs still broken and letting that be sorted out by our URL mapping fun.

> But, weirdly, my machine just started properly restoring chrome:chrome/history halfway through my investigation.

Did you by any chance do a pull? AFAIK creis is working on SessionState things...

@dbeam / comment 14: The last time I talked with groby@, my fix for  issue 638088  was only needed for the case of switching from MD settings mode back to default mode (assuming her draft CL finds a way to land).  It sounded like my part wasn't blocking at the time.

Anyway, I have a draft CL at https://codereview.chromium.org/2316003002/ to fix it, but it feels too risky to land and merge to M54 at the moment.  (I'm not convinced yet if the state stored on RenderFrameImpl is safe/correct.)

I need to run to a meeting, but I can chat more about status later if that's helpful.

blocks M55 (!) beta

sorry for the confusion, all

Hmm. So it looks like session restore when turning on MD History will work, but cause the page to load in classic history.

Once groby's patch is applied, it will actually break unless the rewriting matches whether MD History is actually on or off.

I think restoring to the old history page is fine. Users that have session restore on will just get the new history page the next time they open a fresh history tab.

Once grouped history is launchable, we can remove the profile dependency in the URL rewriting and all will be well.

WDYT?

i think ^ that's shaky at best.

if we had to turn OFF material design history for some reason (i.e. roll back the finch config), it wouldn't work.

0) ./chrome --enable-features=MaterialDesignHistory
1) visit chrome://history
2) quit chrome
3) ./chrome --disable-features=MaterialDesignHistory

shows a broken page.

reloading/duplicating an old UI with new flag enabled somehow works, but you get a different result if you press enter in the omnibox on the same URL (edge casey but weird).  also, having multiple versions of the same page running for the same instance of chrome would be really scary or confusing as a user.  we already got reports of users thinking new UIs are malware, I think stuff like this might contribute to that.

In that case, the only way I can see to get restore working properly on history is to run the MD history UI under chrome://chrome/history until we can get rid of the Profile from IsEnabled().

Here's a preview of what that would look like.

https://codereview.chromium.org/2352553002

I'm a bit worried about registering 2 different UIs for the same host, but it looks like it re-registers every time a WebUI gets created anyway...

This seems to work for most cases, except for the MD History => Old History version for some reason. I'm really quite stuck on that one...

This patch

https://codereview.chromium.org/2355543003/

also seems to work. It's similar to groby's patch but has the added benefit of not needing to go through the URL rewriting.

Applying creis's patch fixes going from MD History to Old History.

Verified the issue on Mac 10.11.6,Win 10 and Ubuntu 14.04 using # 55.0.2868.0 and able to navigate to old chrome://history page when disabled "MaterialDesignHistory" flag and relaunched Chrome.Attached the screen cast for the same.

Added respective TE-Verified labels for the same.
calamity@ : Could you please mark the issue to fixed if there is no further work to be done on this issue.

Deleted: 614758_Sept_22.mp4 704 KB

	614758_Sept_22.mp4 704 KB View Download

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/815b4051771d29afb0f6af14724cad9a620fe720

commit 815b4051771d29afb0f6af14724cad9a620fe720
Author: calamity <calamity@chromium.org>
Date: Fri Sep 23 11:37:25 2016

Invalidate the page state when restoring WebUIs

This CL invalidates the serialized page state any chrome:// URLs,
causing the page to get reloaded rather than restored. This will prevent
the upgrade to MD History from breaking things since it lives at a
different physical URL than classic history.

BUG= 614758 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2355543003
Cr-Commit-Position: refs/heads/master@{#420600}

[modify] https://crrev.com/815b4051771d29afb0f6af14724cad9a620fe720/chrome/browser/sessions/better_session_restore_browsertest.cc
[modify] https://crrev.com/815b4051771d29afb0f6af14724cad9a620fe720/chrome/browser/ui/webui/md_history_ui.cc
[modify] https://crrev.com/815b4051771d29afb0f6af14724cad9a620fe720/chrome/browser/ui/webui/md_history_ui.h
[modify] https://crrev.com/815b4051771d29afb0f6af14724cad9a620fe720/components/sessions/content/content_serialized_navigation_driver.cc
[modify] https://crrev.com/815b4051771d29afb0f6af14724cad9a620fe720/content/public/common/url_constants.cc
[modify] https://crrev.com/815b4051771d29afb0f6af14724cad9a620fe720/content/public/common/url_constants.h

Verified the issue on windows 10, Ubuntu 14.04 and Mac 10.11.6 using chrome dev version #55.0.2873.0 as per the comment #0 and #27
Observed that the fix is working as expected.

Attaching screencast for reference

Deleted: 614758.mp4 2.6 MB

	614758.mp4 2.6 MB View Download

This bug is reported as M55 Beta blocker.Please try to resolve this before M55 branch on Oct 6th,2016 so it has enough baking time in Dev.

calamity@: Is this fixed after r420600?

Yup, this is now fixed.

[Auto-generated comment by a script] We noticed that this issue is targeted for M-55; it appears the fix may have landed after branch point, meaning a merge might be required. Please confirm if a merge is required here - if so add Merge-Request-55 label, otherwise remove Merge-TBD label. Thanks.

CL listed at #31 landed before M55 branch. No M55 merge is needed.