Change the RecentTabHelper to only capture the last one page in a tab.

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}

[Dmitry Titov, 2016-06-24 02:20:18]

dimich@chromium.org changed reviewers:
+ fgorski@chromium.org

[fgorski, 2016-06-24 17:49:20]

code looks good, but we could use a test.

https://codereview.chromium.org/2092983002/diff/1/chrome/browser/android/offl...
File chrome/browser/android/offline_pages/recent_tab_helper.cc (right):

https://codereview.chromium.org/2092983002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/recent_tab_helper.cc:103: 
nit: remove the empty line, please.

https://codereview.chromium.org/2092983002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/recent_tab_helper.cc:150: ClientId
RecentTabHelper::client_id() const {
could we please add client_id() verification in the unit tests?
It does not need to be exposing the method, but if it could simulate 2 tabs and
make sure that there is at most one copy per tab it would probably be supportive
of the cause.

https://codereview.chromium.org/2092983002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/recent_tab_helper.cc:153: return
ClientId(kLastNNamespace, "");
Please document what behavior you expect here. E.g.

// If we cannot get Tab ID, we are reverting to Last N == 1 globally.

Or something like that.

[dewittj, 2016-06-24 17:55:45]

dewittj@chromium.org changed reviewers:
+ dewittj@chromium.org

[dewittj, 2016-06-24 17:55:46]

https://codereview.chromium.org/2092983002/diff/1/chrome/browser/android/offl...
File chrome/browser/android/offline_pages/recent_tab_helper.cc (right):

https://codereview.chromium.org/2092983002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/recent_tab_helper.cc:150: ClientId
RecentTabHelper::client_id() const {
nit: This should be renamed GetClientId since it is no longer a trivial
function.

https://codereview.chromium.org/2092983002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/recent_tab_helper.cc:153: return
ClientId(kLastNNamespace, "");
On 2016/06/24 17:49:20, fgorski wrote:
> Please document what behavior you expect here. E.g.
> 
> // If we cannot get Tab ID, we are reverting to Last N == 1 globally.
> 
> Or something like that.

Is there a case where we can't get tab ID?  If so it might be more prudent to
just disable this feature there.

[Dmitry Titov, 2016-06-24 22:10:41]

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
> RecentTabHelper::client_id() const {
> nit: This should be renamed GetClientId since it is no longer a trivial
> function.

No again a trivial function...

> 
>
https://codereview.chromium.org/2092983002/diff/1/chrome/browser/android/offl...
> chrome/browser/android/offline_pages/recent_tab_helper.cc:153: return
> ClientId(kLastNNamespace, "");
> On 2016/06/24 17:49:20, fgorski wrote:
> > Please document what behavior you expect here. E.g.
> > 
> > // If we cannot get Tab ID, we are reverting to Last N == 1 globally.
> > 
> > Or something like that.
> 
> Is there a case where we can't get tab ID?  If so it might be more prudent to
> just disable this feature there.

Done, turn off capture is no id. We likely are not going to see this on Android,
but it's hard to derive this guarantee from API that exists.

[dewittj, 2016-06-24 23:10:33]

https://codereview.chromium.org/2092983002/diff/20001/chrome/browser/android/...
File chrome/browser/android/offline_pages/recent_tab_helper.cc (right):

https://codereview.chromium.org/2092983002/diff/20001/chrome/browser/android/...
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/...
chrome/browser/android/offline_pages/recent_tab_helper.cc:45: bool GetTabId(int*
tab_id, content::WebContents* web_contents) override {
nit: outparams usually go after inparams in my experience. 

Also, consider base::Optional :)

https://codereview.chromium.org/2092983002/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/recent_tab_helper.cc:46: *tab_id = 0;
nit: don't set tab_id on failure.

https://codereview.chromium.org/2092983002/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/recent_tab_helper.cc:71:
DCHECK(delegate.get());
I think you can just use the boolean conversion:
DCHECK(delegate);

https://codereview.chromium.org/2092983002/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/recent_tab_helper.cc:163:
delegate_->CreatePageArchiver(web_contents()));
nit: move this right into the SavePage call.

[Dmitry Titov, 2016-06-24 23:58:01]

https://codereview.chromium.org/2092983002/diff/20001/chrome/browser/android/...
File chrome/browser/android/offline_pages/recent_tab_helper.cc (right):

https://codereview.chromium.org/2092983002/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/recent_tab_helper.cc:40: return archiver;
On 2016/06/24 23:10:33, dewittj wrote:
> nit:
> return base::MakeUnique<offline_pages::OffinePageArchiver>(web_contents);

Done.

https://codereview.chromium.org/2092983002/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/recent_tab_helper.cc:45: bool GetTabId(int*
tab_id, content::WebContents* web_contents) override {
On 2016/06/24 23:10:33, dewittj wrote:
> nit: outparams usually go after inparams in my experience. 
> 
> Also, consider base::Optional :)

Done. Not Optional...

https://codereview.chromium.org/2092983002/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/recent_tab_helper.cc:46: *tab_id = 0;
On 2016/06/24 23:10:33, dewittj wrote:
> nit: don't set tab_id on failure.

Done.

https://codereview.chromium.org/2092983002/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/recent_tab_helper.cc:71:
DCHECK(delegate.get());
On 2016/06/24 23:10:33, dewittj wrote:
> I think you can just use the boolean conversion:
> DCHECK(delegate);

Done.

https://codereview.chromium.org/2092983002/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/recent_tab_helper.cc:163:
delegate_->CreatePageArchiver(web_contents()));
On 2016/06/24 23:10:33, dewittj wrote:
> nit: move this right into the SavePage call.

Done.

[dewittj, 2016-06-25 00:01:45]

lgtm, thanks!

[Dmitry Titov, 2016-06-25 00:08:15]

The CQ bit was checked by dimich@chromium.org

[Dmitry Titov, 2016-06-25 00:08:26]

The CQ bit was unchecked by dimich@chromium.org

[Dmitry Titov, 2016-06-25 00:21:13]

The CQ bit was checked by dimich@chromium.org

[Dmitry Titov, 2016-06-25 00:21:14]

The patchset sent to the CQ was uploaded after l-g-t-m from dewittj@chromium.org
Link to the patchset: https://codereview.chromium.org/2092983002/#ps60001
(title: "removed unused variable")

[commit-bot: I haz the power, 2016-06-25 00:21:39]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-25 01:07:04]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-06-25 01:09:43]

Description was changed from

==========
Change the RecentTabHelper to only capture the last one page in a tab.

BUG=622424
==========

to

==========
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}
==========

[commit-bot: I haz the power, 2016-06-25 01:09:44]

Patchset 4 (id:??) landed as
https://crrev.com/d8379f4175799d566a7a80bdb05a80f91bcfd2bd
Cr-Commit-Position: refs/heads/master@{#402036}