Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(26)

Issue 2759333002: Move chrome-specific SerializedNavigation code to chrome/. (Closed)

Created:
3 years, 9 months ago by Lei Zhang
Modified:
3 years, 9 months ago
CC:
chromium-reviews, extensions-reviews_chromium.org, ntp-dev+reviews_chromium.org, jam, Patrick Dubroy, darin-cc_chromium.org, dbeam+watch-history_chromium.org, pam+watch_chromium.org, chromium-apps-reviews_chromium.org, noyau+watch_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Move chrome-specific SerializedNavigation code to chrome/. BUG=701605 Review-Url: https://codereview.chromium.org/2759333002 Cr-Commit-Position: refs/heads/master@{#458985} Committed: https://chromium.googlesource.com/chromium/src/+/c839b55b83437730066953a2bdbfef8972ef690c

Patch Set 1 #

Patch Set 2 : Android build #

Patch Set 3 : Fix tests #

Patch Set 4 : TEST_F #

Patch Set 5 : Rebase, fix Android? #

Total comments: 5

Patch Set 6 : ChromeSerializedNavigationDriver #

Patch Set 7 : bad export #

Total comments: 4

Patch Set 8 : rebase, nits #

Patch Set 9 : Fix Android #

Unified diffs Side-by-side diffs Delta from patch set Stats (+343 lines, -215 lines) Patch
M chrome/browser/BUILD.gn View 1 2 3 4 5 6 7 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/browser/android/ntp/new_tab_page_url_handler.cc View 1 1 chunk +2 lines, -2 lines 0 comments Download
M chrome/browser/browser_about_handler.cc View 3 chunks +3 lines, -4 lines 0 comments Download
M chrome/browser/chrome_browser_main.cc View 1 2 3 4 5 6 7 8 2 chunks +8 lines, -0 lines 0 comments Download
M chrome/browser/chrome_content_browser_client.cc View 1 2 3 4 5 6 7 2 chunks +1 line, -2 lines 0 comments Download
A chrome/browser/sessions/chrome_serialized_navigation_driver.h View 1 2 3 4 5 6 1 chunk +40 lines, -0 lines 0 comments Download
A chrome/browser/sessions/chrome_serialized_navigation_driver.cc View 1 2 3 4 5 6 7 1 chunk +103 lines, -0 lines 0 comments Download
A chrome/browser/sessions/chrome_serialized_navigation_driver_unittest.cc View 1 2 3 4 5 6 7 1 chunk +109 lines, -0 lines 0 comments Download
M chrome/browser/sessions/session_common_utils_unittest.cc View 1 2 3 4 5 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/ui/browser_navigator.cc View 2 chunks +1 line, -2 lines 0 comments Download
M chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc View 1 2 3 4 5 6 7 3 chunks +2 lines, -3 lines 0 comments Download
M chrome/browser/ui/webui/md_history_ui.cc View 3 chunks +2 lines, -3 lines 0 comments Download
M chrome/browser/ui/webui/uber/uber_ui.cc View 5 chunks +6 lines, -7 lines 0 comments Download
M chrome/common/extensions/chrome_manifest_url_handlers.cc View 2 chunks +1 line, -2 lines 0 comments Download
M chrome/common/url_constants.h View 1 2 3 4 5 6 7 2 chunks +2 lines, -0 lines 0 comments Download
M chrome/common/url_constants.cc View 1 2 3 4 5 6 7 4 chunks +3 lines, -1 line 0 comments Download
M chrome/test/BUILD.gn View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download
M components/sessions/content/content_serialized_navigation_driver.h View 1 2 3 4 5 3 chunks +7 lines, -4 lines 0 comments Download
M components/sessions/content/content_serialized_navigation_driver.cc View 1 2 3 4 5 3 chunks +23 lines, -73 lines 0 comments Download
M components/sessions/content/content_serialized_navigation_driver_unittest.cc View 1 2 2 chunks +0 lines, -79 lines 0 comments Download
M components/sessions/core/serialized_navigation_driver.h View 1 2 3 4 5 2 chunks +3 lines, -2 lines 0 comments Download
M components/sessions/core/serialized_navigation_entry.h View 3 chunks +23 lines, -6 lines 0 comments Download
M content/public/common/url_constants.h View 4 chunks +0 lines, -12 lines 0 comments Download
M content/public/common/url_constants.cc View 4 chunks +0 lines, -13 lines 0 comments Download

Messages

Total messages: 69 (42 generated)
Lei Zhang
3 years, 9 months ago (2017-03-21 05:37:42 UTC) #14
Dan Beam
https://codereview.chromium.org/2759333002/diff/80001/components/sessions/content/content_serialized_navigation_driver.cc File components/sessions/content/content_serialized_navigation_driver.cc (right): https://codereview.chromium.org/2759333002/diff/80001/components/sessions/content/content_serialized_navigation_driver.cc#newcode107 components/sessions/content/content_serialized_navigation_driver.cc:107: if (callback_) do you need this if?
3 years, 9 months ago (2017-03-21 05:47:50 UTC) #17
Dan Beam
otherwise this looks awesome, I'm just mildly worried that this callback not being set might ...
3 years, 9 months ago (2017-03-21 05:48:34 UTC) #18
sky
https://codereview.chromium.org/2759333002/diff/80001/components/sessions/core/serialized_navigation_driver.h File components/sessions/core/serialized_navigation_driver.h (right): https://codereview.chromium.org/2759333002/diff/80001/components/sessions/core/serialized_navigation_driver.h#newcode22 components/sessions/core/serialized_navigation_driver.h:22: using SanitizeCallback = base::Callback<void(SerializedNavigationEntry*)>; This seems like a roundabout ...
3 years, 9 months ago (2017-03-21 14:47:39 UTC) #19
Lei Zhang
https://codereview.chromium.org/2759333002/diff/80001/components/sessions/content/content_serialized_navigation_driver.cc File components/sessions/content/content_serialized_navigation_driver.cc (right): https://codereview.chromium.org/2759333002/diff/80001/components/sessions/content/content_serialized_navigation_driver.cc#newcode107 components/sessions/content/content_serialized_navigation_driver.cc:107: if (callback_) On 2017/03/21 05:47:50, Dan Beam wrote: > ...
3 years, 9 months ago (2017-03-21 17:12:51 UTC) #20
sky
https://codereview.chromium.org/2759333002/diff/80001/components/sessions/core/serialized_navigation_driver.h File components/sessions/core/serialized_navigation_driver.h (right): https://codereview.chromium.org/2759333002/diff/80001/components/sessions/core/serialized_navigation_driver.h#newcode22 components/sessions/core/serialized_navigation_driver.h:22: using SanitizeCallback = base::Callback<void(SerializedNavigationEntry*)>; On 2017/03/21 17:12:51, Lei Zhang ...
3 years, 9 months ago (2017-03-21 17:50:59 UTC) #21
gone
Is there anything in particular I'm supposed to be looking at here?
3 years, 9 months ago (2017-03-21 17:54:26 UTC) #22
Lei Zhang
On 2017/03/21 17:54:26, dfalcantara (load balance plz) wrote: > Is there anything in particular I'm ...
3 years, 9 months ago (2017-03-21 18:00:26 UTC) #23
Lei Zhang
On 2017/03/21 17:50:59, sky wrote: > > Wouldn't I need to move SerializedNavigationDriver::Get() and > ...
3 years, 9 months ago (2017-03-21 18:41:24 UTC) #24
gone
On 2017/03/21 18:00:26, Lei Zhang (super slow) wrote: > On 2017/03/21 17:54:26, dfalcantara (load balance ...
3 years, 9 months ago (2017-03-21 18:53:47 UTC) #25
gone
+dtrainor because he probably understands this pathway better than I do
3 years, 9 months ago (2017-03-21 18:54:24 UTC) #27
sky
On Tue, Mar 21, 2017 at 11:41 AM, <thestig@chromium.org> wrote: > On 2017/03/21 17:50:59, sky ...
3 years, 9 months ago (2017-03-21 20:58:09 UTC) #28
Lei Zhang
PTAL at patch set 7. There are some red trybots but I'm not certain if ...
3 years, 9 months ago (2017-03-22 01:29:19 UTC) #37
sky
LGTM https://codereview.chromium.org/2759333002/diff/120001/chrome/browser/sessions/chrome_serialized_navigation_driver.cc File chrome/browser/sessions/chrome_serialized_navigation_driver.cc (right): https://codereview.chromium.org/2759333002/diff/120001/chrome/browser/sessions/chrome_serialized_navigation_driver.cc#newcode103 chrome/browser/sessions/chrome_serialized_navigation_driver.cc:103: ChromeSerializedNavigationDriver::~ChromeSerializedNavigationDriver() {} Make Definition/declaration order match. https://codereview.chromium.org/2759333002/diff/120001/chrome/browser/sessions/chrome_serialized_navigation_driver_unittest.cc File ...
3 years, 9 months ago (2017-03-22 04:00:31 UTC) #38
Lei Zhang
https://codereview.chromium.org/2759333002/diff/120001/chrome/browser/sessions/chrome_serialized_navigation_driver.cc File chrome/browser/sessions/chrome_serialized_navigation_driver.cc (right): https://codereview.chromium.org/2759333002/diff/120001/chrome/browser/sessions/chrome_serialized_navigation_driver.cc#newcode103 chrome/browser/sessions/chrome_serialized_navigation_driver.cc:103: ChromeSerializedNavigationDriver::~ChromeSerializedNavigationDriver() {} On 2017/03/22 04:00:31, sky wrote: > Make ...
3 years, 9 months ago (2017-03-22 05:59:05 UTC) #39
Lei Zhang
+jam for content/OWNERS - context: https://codereview.chromium.org/2743323005/
3 years, 9 months ago (2017-03-22 06:02:52 UTC) #43
jam
lgtm, thanks!
3 years, 9 months ago (2017-03-22 15:02:32 UTC) #46
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2759333002/140001
3 years, 9 months ago (2017-03-22 19:01:50 UTC) #49
commit-bot: I haz the power
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/391853)
3 years, 9 months ago (2017-03-22 19:11:36 UTC) #51
Dan Beam
lgtm
3 years, 9 months ago (2017-03-22 20:28:20 UTC) #52
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2759333002/140001
3 years, 9 months ago (2017-03-22 20:29:18 UTC) #54
Lei Zhang
On 2017/03/22 20:29:18, commit-bot: I haz the power wrote: > CQ is trying da patch. ...
3 years, 9 months ago (2017-03-22 20:31:12 UTC) #55
commit-bot: I haz the power
Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_swarming_rel/builds/141952)
3 years, 9 months ago (2017-03-22 22:11:07 UTC) #57
gone
If it's offline pages stuff that's mucking with the startup flow, I'd poke dtrainor@ or ...
3 years, 9 months ago (2017-03-23 00:18:50 UTC) #60
Lei Zhang
On 2017/03/23 00:18:50, dfalcantara (load balance plz) wrote: > If it's offline pages stuff that's ...
3 years, 9 months ago (2017-03-23 00:32:32 UTC) #61
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2759333002/160001
3 years, 9 months ago (2017-03-23 02:24:25 UTC) #66
commit-bot: I haz the power
3 years, 9 months ago (2017-03-23 02:39:50 UTC) #69
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/c839b55b83437730066953a2bdbf...

Powered by Google App Engine
This is Rietveld 408576698