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

Issue 2092983002: Change the RecentTabHelper to only capture the last one page in a tab. (Closed)

Created:
4 years, 6 months ago by Dmitry Titov
Modified:
4 years, 5 months ago
Reviewers:
fgorski, dewittj
CC:
chromium-reviews, romax+watch_chromium.org, fgorski+watch_chromium.org, dewittj+watch_chromium.org, petewil+watch_chromium.org, chili+watch_chromium.org, dimich+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Change the RecentTabHelper to only capture the last one page in a tab. BUG=622424 Committed: https://crrev.com/d8379f4175799d566a7a80bdb05a80f91bcfd2bd Cr-Commit-Position: refs/heads/master@{#402036}

Patch Set 1 #

Total comments: 5

Patch Set 2 : added unittests, process 'no id' case #

Total comments: 10

Patch Set 3 : addressed feedback, fixed bug (added LazyInit) #

Patch Set 4 : removed unused variable #

Unified diffs Side-by-side diffs Delta from patch set Stats (+169 lines, -104 lines) Patch
M chrome/browser/android/offline_pages/recent_tab_helper.h View 1 2 2 chunks +19 lines, -9 lines 0 comments Download
M chrome/browser/android/offline_pages/recent_tab_helper.cc View 1 2 6 chunks +58 lines, -78 lines 0 comments Download
M chrome/browser/android/offline_pages/recent_tab_helper_unittest.cc View 1 2 3 9 chunks +92 lines, -17 lines 0 comments Download

Messages

Total messages: 17 (7 generated)
Dmitry Titov
4 years, 6 months ago (2016-06-24 02:20:19 UTC) #2
fgorski
code looks good, but we could use a test. https://codereview.chromium.org/2092983002/diff/1/chrome/browser/android/offline_pages/recent_tab_helper.cc File chrome/browser/android/offline_pages/recent_tab_helper.cc (right): https://codereview.chromium.org/2092983002/diff/1/chrome/browser/android/offline_pages/recent_tab_helper.cc#newcode103 chrome/browser/android/offline_pages/recent_tab_helper.cc:103: ...
4 years, 5 months ago (2016-06-24 17:49:20 UTC) #3
dewittj
https://codereview.chromium.org/2092983002/diff/1/chrome/browser/android/offline_pages/recent_tab_helper.cc File chrome/browser/android/offline_pages/recent_tab_helper.cc (right): https://codereview.chromium.org/2092983002/diff/1/chrome/browser/android/offline_pages/recent_tab_helper.cc#newcode150 chrome/browser/android/offline_pages/recent_tab_helper.cc:150: ClientId RecentTabHelper::client_id() const { nit: This should be renamed ...
4 years, 5 months ago (2016-06-24 17:55:46 UTC) #5
Dmitry Titov
PTAL, added couple unit tests and fixed: On 2016/06/24 17:55:46, dewittj wrote: > chrome/browser/android/offline_pages/recent_tab_helper.cc:150: ClientId ...
4 years, 5 months ago (2016-06-24 22:10:41 UTC) #6
dewittj
https://codereview.chromium.org/2092983002/diff/20001/chrome/browser/android/offline_pages/recent_tab_helper.cc File chrome/browser/android/offline_pages/recent_tab_helper.cc (right): https://codereview.chromium.org/2092983002/diff/20001/chrome/browser/android/offline_pages/recent_tab_helper.cc#newcode40 chrome/browser/android/offline_pages/recent_tab_helper.cc:40: return archiver; nit: return base::MakeUnique<offline_pages::OffinePageArchiver>(web_contents); https://codereview.chromium.org/2092983002/diff/20001/chrome/browser/android/offline_pages/recent_tab_helper.cc#newcode45 chrome/browser/android/offline_pages/recent_tab_helper.cc:45: bool GetTabId(int* ...
4 years, 5 months ago (2016-06-24 23:10:33 UTC) #7
Dmitry Titov
https://codereview.chromium.org/2092983002/diff/20001/chrome/browser/android/offline_pages/recent_tab_helper.cc File chrome/browser/android/offline_pages/recent_tab_helper.cc (right): https://codereview.chromium.org/2092983002/diff/20001/chrome/browser/android/offline_pages/recent_tab_helper.cc#newcode40 chrome/browser/android/offline_pages/recent_tab_helper.cc:40: return archiver; On 2016/06/24 23:10:33, dewittj wrote: > nit: ...
4 years, 5 months ago (2016-06-24 23:58:01 UTC) #8
dewittj
lgtm, thanks!
4 years, 5 months ago (2016-06-25 00:01:45 UTC) #9
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/2092983002/60001
4 years, 5 months ago (2016-06-25 00:21:39 UTC) #14
commit-bot: I haz the power
Committed patchset #4 (id:60001)
4 years, 5 months ago (2016-06-25 01:07:04 UTC) #15
commit-bot: I haz the power
4 years, 5 months ago (2016-06-25 01:09:44 UTC) #17
Message was sent while issue was closed.
Patchset 4 (id:??) landed as
https://crrev.com/d8379f4175799d566a7a80bdb05a80f91bcfd2bd
Cr-Commit-Position: refs/heads/master@{#402036}

Powered by Google App Engine
This is Rietveld 408576698