[TabManager] Move remaining discard logic from TabStripModel to TabManager.

[TabManager] Move remaining discard logic from TabStripModel to TabManager.

This CL removes the remaining code related to discarding from TabStripModel to TabManager. It also cleans up the API exposed for discarding.

Note that we still have one pending code in browser.cc that I'll investigate independently, as I'm not even convinced of it's usefulness.

BUG=539504
Committed: https://crrev.com/f6877bd6d5adfec08799077baa3781bc41b3c12e
Cr-Commit-Position: refs/heads/master@{#357115}

[Georges Khalil, 2015-10-27 17:47:47]

Description was changed from

==========
[TabManager] Move remaining discard logic from TabStripModel to TabManager.

BUG=539504
==========

to

==========
[TabManager] Move remaining discard logic from TabStripModel to TabManager.

This CL removes the remaining code related to discarding from TabStripModel to
TabManager. It also cleans up the API exposed for discarding.

Note that we still have one pending code in browser.cc that I'll investigate
independently, as I'm not even convinced of it's usefulness.

BUG=539504
==========

[Georges Khalil, 2015-10-27 17:58:43]

Patchset #1 (id:1) has been deleted

[Georges Khalil, 2015-10-27 17:58:49]

Patchset #1 (id:20001) has been deleted

[Georges Khalil, 2015-10-27 17:58:57]

Patchset #1 (id:40001) has been deleted

[Georges Khalil, 2015-10-27 18:09:42]

georgesak@chromium.org changed reviewers:
+ chrisha@chromium.org, sky@chromium.org

[Georges Khalil, 2015-10-27 18:09:43]

chrisha@, sky@, PTAL.

[sky, 2015-10-27 21:16:46]

https://codereview.chromium.org/1427613002/diff/60001/chrome/browser/memory/t...
File chrome/browser/memory/tab_manager.cc (right):

https://codereview.chromium.org/1427613002/diff/60001/chrome/browser/memory/t...
chrome/browser/memory/tab_manager.cc:232:
WebContentsData::CreateForWebContents(contents);
Is this necessary? Doesn't GetWebContentsData() implicitly create?

https://codereview.chromium.org/1427613002/diff/60001/chrome/browser/memory/t...
File chrome/browser/memory/tab_manager_web_contents_data.cc (right):

https://codereview.chromium.org/1427613002/diff/60001/chrome/browser/memory/t...
chrome/browser/memory/tab_manager_web_contents_data.cc:21:
WebContentsData::FromWebContents(web_contents())->SetDiscardState(false);
Isn't WebContentsData::FromWebContents(web_contents()) the same as this?

https://codereview.chromium.org/1427613002/diff/60001/chrome/browser/memory/t...
File chrome/browser/memory/tab_manager_web_contents_data.h (right):

https://codereview.chromium.org/1427613002/diff/60001/chrome/browser/memory/t...
chrome/browser/memory/tab_manager_web_contents_data.h:27: :
WebContentsObserver(web_contents) {}
Don't inline this, and I wouldn't inline the destructor either.

[Georges Khalil, 2015-10-30 14:17:45]

sky@, PTAnL.

https://codereview.chromium.org/1427613002/diff/60001/chrome/browser/memory/t...
File chrome/browser/memory/tab_manager.cc (right):

https://codereview.chromium.org/1427613002/diff/60001/chrome/browser/memory/t...
chrome/browser/memory/tab_manager.cc:232:
WebContentsData::CreateForWebContents(contents);
On 2015/10/27 21:16:46, sky wrote:
> Is this necessary? Doesn't GetWebContentsData() implicitly create?

Correct, this is not needed. Removed.

https://codereview.chromium.org/1427613002/diff/60001/chrome/browser/memory/t...
File chrome/browser/memory/tab_manager_web_contents_data.cc (right):

https://codereview.chromium.org/1427613002/diff/60001/chrome/browser/memory/t...
chrome/browser/memory/tab_manager_web_contents_data.cc:21:
WebContentsData::FromWebContents(web_contents())->SetDiscardState(false);
On 2015/10/27 21:16:46, sky wrote:
> Isn't WebContentsData::FromWebContents(web_contents()) the same as this?

Yep. SetDiscardedState was static at first but I changed it after and forgot to
update this, good catch.

Done.

https://codereview.chromium.org/1427613002/diff/60001/chrome/browser/memory/t...
File chrome/browser/memory/tab_manager_web_contents_data.h (right):

https://codereview.chromium.org/1427613002/diff/60001/chrome/browser/memory/t...
chrome/browser/memory/tab_manager_web_contents_data.h:27: :
WebContentsObserver(web_contents) {}
On 2015/10/27 21:16:46, sky wrote:
> Don't inline this, and I wouldn't inline the destructor either.

Done.

[Georges Khalil, 2015-10-30 14:43:11]

Patchset #3 (id:100001) has been deleted

[Georges Khalil, 2015-10-30 14:44:31]

Patchset #3 (id:120001) has been deleted

[sky, 2015-10-30 15:14:38]

LGTM

[chrisha, 2015-10-30 15:24:40]

This is much cleaner!

lgtm

https://codereview.chromium.org/1427613002/diff/140001/chrome/browser/memory/...
File chrome/browser/memory/tab_manager.cc (right):

https://codereview.chromium.org/1427613002/diff/140001/chrome/browser/memory/...
chrome/browser/memory/tab_manager.cc:235: auto data =
GetWebContentsData(contents);
Doesn't this already call create CreateForWebContents? So need to also call it
above?

[Georges Khalil, 2015-10-30 15:30:05]

Thanks to both of you.

Sending it to CQ.

https://codereview.chromium.org/1427613002/diff/140001/chrome/browser/memory/...
File chrome/browser/memory/tab_manager.cc (right):

https://codereview.chromium.org/1427613002/diff/140001/chrome/browser/memory/...
chrome/browser/memory/tab_manager.cc:235: auto data =
GetWebContentsData(contents);
On 2015/10/30 15:24:40, chrisha wrote:
> Doesn't this already call create CreateForWebContents? So need to also call it
> above?

Removed the call to CreateForWebContents at start of function.

[Georges Khalil, 2015-10-30 15:30:22]

The CQ bit was checked by georgesak@chromium.org

[Georges Khalil, 2015-10-30 15:30:22]

The patchset sent to the CQ was uploaded after l-g-t-m from
chrisha@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1427613002/#ps160001
(title: "chrisha@ comments.")

[commit-bot: I haz the power, 2015-10-30 15:30:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1427613002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1427613002/160001

[commit-bot: I haz the power, 2015-10-30 16:10:44]

Committed patchset #4 (id:160001)

[commit-bot: I haz the power, 2015-10-30 16:11:20]

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