New Public IdFromWebContents method on TabManager.

New Public IdFromWebContents method on TabManager.

Increase readbility by making the method public and therefore clients of TabManager don't need to know how the ids are created.

BUG=621960
Committed: https://crrev.com/3b55735a3115817131690dc0607a6f998de23717
Cr-Commit-Position: refs/heads/master@{#412386}

[Anderson Silva, 2016-08-16 18:05:16]

The CQ bit was checked by andersoncss@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-16 18:05:34]

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

[Anderson Silva, 2016-08-16 18:06:03]

Description was changed from

==========
New Public IdFromWebContents method on TabManager.

Increase readbility by making the method public and therefore clients fo
TabManager don't need to know how the ids are created.

BUG=621960
==========

to

==========
New Public IdFromWebContents method on TabManager.

Increase readbility by making the method public and therefore clients fo
TabManager don't need to know how the ids are created.

BUG=621960
==========

[Anderson Silva, 2016-08-16 18:06:03]

andersoncss@google.com changed reviewers:
+ chrisha@chromium.org, georgesak@chromium.org

[Anderson Silva, 2016-08-16 18:06:26]

cleaning up my bug list!
ptal

[commit-bot: I haz the power, 2016-08-16 19:03:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-16 19:03:41]

Dry run: This issue passed the CQ dry run.

[Georges Khalil, 2016-08-16 21:18:29]

lgtm % nit

https://codereview.chromium.org/2250183002/diff/1/chrome/browser/memory/tab_m...
File chrome/browser/memory/tab_manager.cc (right):

https://codereview.chromium.org/2250183002/diff/1/chrome/browser/memory/tab_m...
chrome/browser/memory/tab_manager.cc:349: return
DiscardTabById(TabManager::IdFromWebContents(contents));
nit: remove TabManager::

[Anderson Silva, 2016-08-16 21:36:49]

Description was changed from

==========
New Public IdFromWebContents method on TabManager.

Increase readbility by making the method public and therefore clients fo
TabManager don't need to know how the ids are created.

BUG=621960
==========

to

==========
New Public IdFromWebContents method on TabManager.

Increase readbility by making the method public and therefore clients of
TabManager don't need to know how the ids are created.

BUG=621960
==========

[Anderson Silva, 2016-08-16 21:37:24]

andersoncss@google.com changed reviewers:
+ rdevlin.cronin@chromium.org

[Anderson Silva, 2016-08-16 21:37:51]

Hi Devlin, can you please take a look at this CL?
Thanks

https://codereview.chromium.org/2250183002/diff/1/chrome/browser/memory/tab_m...
File chrome/browser/memory/tab_manager.cc (right):

https://codereview.chromium.org/2250183002/diff/1/chrome/browser/memory/tab_m...
chrome/browser/memory/tab_manager.cc:349: return
DiscardTabById(TabManager::IdFromWebContents(contents));
On 2016/08/16 21:18:29, Georges Khalil (OOO Sep 7) wrote:
> nit: remove TabManager::

Done.

[Devlin, 2016-08-16 22:56:06]

lgtm with an optional suggestion

https://codereview.chromium.org/2250183002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/api/tabs/tabs_test.cc (right):

https://codereview.chromium.org/2250183002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_test.cc:1364:
memory::TabManager::IdFromWebContents(web_contents_a)));
We could make this even easier by providing a
DiscardTab[WithWebContents](content::WebContents*) method in TabManager, so that
callers don't even need to go through this effort.  WDYT?

[Anderson Silva, 2016-08-16 23:13:31]

thanks! submitting it to CQ

https://codereview.chromium.org/2250183002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/api/tabs/tabs_test.cc (right):

https://codereview.chromium.org/2250183002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_test.cc:1364:
memory::TabManager::IdFromWebContents(web_contents_a)));
On 2016/08/16 22:56:06, Devlin wrote:
> We could make this even easier by providing a
> DiscardTab[WithWebContents](content::WebContents*) method in TabManager, so
that
> callers don't even need to go through this effort.  WDYT?

Done. You're right, I've even already implemented such function.

[Anderson Silva, 2016-08-16 23:13:34]

The CQ bit was checked by andersoncss@google.com

[Anderson Silva, 2016-08-16 23:13:34]

The patchset sent to the CQ was uploaded after l-g-t-m from
georgesak@chromium.org, rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/2250183002/#ps40001
(title: "implemented suggestion")

[commit-bot: I haz the power, 2016-08-16 23:14:18]

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-08-16 23:59:58]

Description was changed from

==========
New Public IdFromWebContents method on TabManager.

Increase readbility by making the method public and therefore clients of
TabManager don't need to know how the ids are created.

BUG=621960
==========

to

==========
New Public IdFromWebContents method on TabManager.

Increase readbility by making the method public and therefore clients of
TabManager don't need to know how the ids are created.

BUG=621960
==========

[commit-bot: I haz the power, 2016-08-17 00:00:00]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-08-17 00:01:58]

Description was changed from

==========
New Public IdFromWebContents method on TabManager.

Increase readbility by making the method public and therefore clients of
TabManager don't need to know how the ids are created.

BUG=621960
==========

to

==========
New Public IdFromWebContents method on TabManager.

Increase readbility by making the method public and therefore clients of
TabManager don't need to know how the ids are created.

BUG=621960

Committed: https://crrev.com/3b55735a3115817131690dc0607a6f998de23717
Cr-Commit-Position: refs/heads/master@{#412386}
==========

[commit-bot: I haz the power, 2016-08-17 00:01:59]

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

[Devlin, 2016-08-17 00:02:44]

non-blocking drive-by - feel free to ignore or fix in a followup.

https://codereview.chromium.org/2250183002/diff/40001/chrome/browser/memory/t...
File chrome/browser/memory/tab_manager.h (right):

https://codereview.chromium.org/2250183002/diff/40001/chrome/browser/memory/t...
chrome/browser/memory/tab_manager.h:115: content::WebContents*
DiscardTabByExtension(content::WebContents* contents);
drive-by: It might make sense to rename this method to be something like
DiscardTabWithWebContents() so that readers don't assume that either a) it
should only be used by extensions or b) it does some kind of attribution to
extensions.  I'd probably also omit the part about "used by the extensions API"
- method descriptions should typically describe the behavior rather than the
call sites (unless there's a reason to limit the call site, as in cases where we
might say "Only use this if it's attributable to an extension, etc).