New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 701605 link

Starred by 1 user
Status: Fixed
Owner:
thestig@chromium.org
Closed: Mar 2017
Cc:
█ skuhne@chromium.org
dbeam@chromium.org
sky@chromium.org
dpa...@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 1
Type: Bug

Blocking:
issue 700350



Sign in to add a comment

Is ContentSerializedNavigationDriver::Sanitize too Chromium specific?

Project Member Reported by thestig@chromium.org, Mar 15 2017 
ContentSerializedNavigationDriver::Sanitize() contains a bunch of checks for manipulating SerializedNavigationEntry state. Do they make sense when the code is in a component? Should Sanitize() delegate to the embedder?

We copied / moved a bunch of WebUI URL constants from chrome/ to content/ to make the code in Sanitize() work. There's a few that ended up getting hard coded. If we do decide to delegate to the embedder, we should re-examine the WebUI constants, e.g. kChromeUIHistoryHost, and decide if they should live in content/ or chrome/.

Comment 1 by thestig@chromium.org, Mar 15 2017 

BTW, I'm happy to help work on this, given some blessings / guidance.

Comment 2 by sky@chromium.org, Mar 15 2017 

I agree that if you want to move the constants out of content, then ContentSerializedNavigationDriver would have to delegate out as well.

Comment 3 by dbeam@chromium.org, Mar 17 2017

Blocking: 671375
Components: UI>Settings
Labels: -Pri-3 Proj-MaterialDesign-WebUI Hotlist-MD-Settings-Navigation Pri-1

Comment 4 by dbeam@chromium.org, Mar 17 2017

Labels: M-59

Comment 5 by tbuck...@chromium.org, Mar 17 2017

Owner: dbeam@chromium.org
Status: Assigned (was: Untriaged)

Comment 6 by dbeam@chromium.org, Mar 17 2017

Blocking: -671375 700350

Comment 7 by thestig@chromium.org, Mar 17 2017 

Feel free to punt this back to me if you'd like. I'm thinking Chrome should pass in a callback on startup, and ContentSerializedNavigationDriver::Sanitize() will just calls the callback with the SerializedNavigationEntry.

Comment 8 by dbeam@chromium.org, Mar 18 2017

Owner: thestig@chromium.org
thestig@ has selfless volunteered to figure this out.

our tentative plan is to set a callback on this singleton after creation from chrome/ code with knowledge/access to chrome-specific URLs.

Comment 9 by dbeam@chromium.org, Mar 19 2017

Cc: dbeam@chromium.org

Comment 10 by thestig@chromium.org, Mar 21 2017

Status: Started (was: Assigned)
CL uploaded but not quite ready yet: https://codereview.chromium.org/2759333002
Project Member

Comment 11 by bugdroid1@chromium.org, Mar 23 2017 

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

commit c839b55b83437730066953a2bdbfef8972ef690c
Author: thestig <thestig@chromium.org>
Date: Thu Mar 23 02:38:56 2017

Move chrome-specific SerializedNavigation code to chrome/.

BUG= 701605 

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

[modify] https://crrev.com/c839b55b83437730066953a2bdbfef8972ef690c/chrome/browser/BUILD.gn
[modify] https://crrev.com/c839b55b83437730066953a2bdbfef8972ef690c/chrome/browser/android/ntp/new_tab_page_url_handler.cc
[modify] https://crrev.com/c839b55b83437730066953a2bdbfef8972ef690c/chrome/browser/browser_about_handler.cc
[modify] https://crrev.com/c839b55b83437730066953a2bdbfef8972ef690c/chrome/browser/chrome_browser_main.cc
[modify] https://crrev.com/c839b55b83437730066953a2bdbfef8972ef690c/chrome/browser/chrome_content_browser_client.cc
[add] https://crrev.com/c839b55b83437730066953a2bdbfef8972ef690c/chrome/browser/sessions/chrome_serialized_navigation_driver.cc
[add] https://crrev.com/c839b55b83437730066953a2bdbfef8972ef690c/chrome/browser/sessions/chrome_serialized_navigation_driver.h
[add] https://crrev.com/c839b55b83437730066953a2bdbfef8972ef690c/chrome/browser/sessions/chrome_serialized_navigation_driver_unittest.cc
[modify] https://crrev.com/c839b55b83437730066953a2bdbfef8972ef690c/chrome/browser/sessions/session_common_utils_unittest.cc
[modify] https://crrev.com/c839b55b83437730066953a2bdbfef8972ef690c/chrome/browser/ui/browser_navigator.cc
[modify] https://crrev.com/c839b55b83437730066953a2bdbfef8972ef690c/chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc
[modify] https://crrev.com/c839b55b83437730066953a2bdbfef8972ef690c/chrome/browser/ui/webui/md_history_ui.cc
[modify] https://crrev.com/c839b55b83437730066953a2bdbfef8972ef690c/chrome/browser/ui/webui/uber/uber_ui.cc
[modify] https://crrev.com/c839b55b83437730066953a2bdbfef8972ef690c/chrome/common/extensions/chrome_manifest_url_handlers.cc
[modify] https://crrev.com/c839b55b83437730066953a2bdbfef8972ef690c/chrome/common/url_constants.cc
[modify] https://crrev.com/c839b55b83437730066953a2bdbfef8972ef690c/chrome/common/url_constants.h
[modify] https://crrev.com/c839b55b83437730066953a2bdbfef8972ef690c/chrome/test/BUILD.gn
[modify] https://crrev.com/c839b55b83437730066953a2bdbfef8972ef690c/components/sessions/content/content_serialized_navigation_driver.cc
[modify] https://crrev.com/c839b55b83437730066953a2bdbfef8972ef690c/components/sessions/content/content_serialized_navigation_driver.h
[modify] https://crrev.com/c839b55b83437730066953a2bdbfef8972ef690c/components/sessions/content/content_serialized_navigation_driver_unittest.cc
[modify] https://crrev.com/c839b55b83437730066953a2bdbfef8972ef690c/components/sessions/core/serialized_navigation_driver.h
[modify] https://crrev.com/c839b55b83437730066953a2bdbfef8972ef690c/components/sessions/core/serialized_navigation_entry.h
[modify] https://crrev.com/c839b55b83437730066953a2bdbfef8972ef690c/content/public/common/url_constants.cc
[modify] https://crrev.com/c839b55b83437730066953a2bdbfef8972ef690c/content/public/common/url_constants.h

Comment 12 by thestig@chromium.org, Mar 23 2017

Status: Fixed (was: Started)
►

Sign in to add a comment