Use virtualURL in history |
||||||
Issue descriptionUpstream, the URL passed to history is web_contents()->GetURL() https://cs.chromium.org/chromium/src/chrome/browser/history/history_tab_helper.cc?sq=package:chromium&dr=CSs&rcl=1481717748&l=101 But the web_contents()->GetURL() is really badly named as it returns the VirtualURL and not the URL. https://cs.chromium.org/chromium/src/content/browser/web_contents/web_contents_impl.cc?sq=package:chromium&dr=CSs This cause the loading of offline version of reading list to not be added to history.
,
Jan 17 2017
Ping?
,
Jan 23 2017
Ping ? This is RBS. This is blocking Reading List.
,
Jan 30 2017
Ping
,
Jan 30 2017
+Eugene as well. It seems like we should switch to storing virtual URL in history, to match other platforms, but I'm not comfortable changing that in M57. I'll chat with folks in MTV tomorrow about what effect this will have on the history system. For M57: 1) What user-visible effect is there if we do not fix this bug? Is the resulting experience bad enough that this needs to be RBS, or can we keep the current behavior until we figure out how to fix history? 2) If we need to fix, we could potentially update -[Tab addCurrentEntryToHistoryDB] to use a different URL for offline pages. This would scope the change to only affect offline pages, which would lower the risk for M57.
,
Feb 2 2017
Any update?
,
Feb 2 2017
Rohit: Would Reading List be the only place where virtualURL is used? Let us know about your chat with folks in MTV regarding effects on the history system. I discussed with Olivier and I personally don't think this is an RBS for M57 since the end-user exposure is fairly limited but I think it's the right thing to do. I will remove the RBS label but we should address this issue. Thanks.
,
Feb 2 2017
Sorry for silence, but I really have no idea how using VirtualURL will affect history behavior :(
,
Sep 27
This is still an issue, I'm not familiar to VirtualURL's vs URL's, but maybe we can discuss once Rohit is back!
,
Sep 27
VirtualURL is url displayed to the user. URL is url passed to the renderer. Sergio, do you want to take this bug?
,
Sep 27
But why is that going to fix History? Assigning the bug to me, but I'd like to hear from Rohit and/or Olivier. Also lowering the priority since this issue has existed for almost 2 years now
,
Sep 27
Re to comment #11: History UI should use VirtualURL, because VirtualURL's purpose is displaying in UI. We can talk offline if you have further question.
,
Sep 28
I'm fairly certain the only time the virtual URL is different from the actual URL is for distilled reading list articles. We display the original article's URL in the omnibox, but the renderer is actually loading a file:// URL from disk. For history, we want to store navigations to the actual URL, not the distilled URL. I believe our old WebUI implementation also did something similar, where we displayed a chrome:// URL in the omnibox, but actually loaded html using a data URL. |
||||||
►
Sign in to add a comment |
||||||
Comment 1 by olivierrobin@chromium.org
, Dec 29 2016Components: UI>Browser>History
Labels: ReleaseBlock-Stable M-57