Add commands to manage tabs and contexts to Browser Domain

Add commands to manage tabs and contexts to Browser Domain

Browser.newPage and Browser.closePage are usable in both normal chrome
and in headless chrome.  Browser.newContext and Browser.closeContext
are implemented in headless chrome only.

Note Browser.newPage has a number of headless chrome only
parameters: width, height, contextId

BUG=546953
Committed: https://crrev.com/72564fc55f6c9a186fca1055004c1c892d68026d
Cr-Commit-Position: refs/heads/master@{#404099}

[alex clarke (OOO till 29th), 2016-07-03 18:38:51]

The CQ bit was checked by alexclarke@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-03 18:39:00]

Dry run: 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-07-03 18:43:32]

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

[commit-bot: I haz the power, 2016-07-03 18:43:33]

Dry run: Try jobs failed on following builders:
  ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)

[alex clarke (OOO till 29th), 2016-07-03 21:21:56]

The CQ bit was checked by alexclarke@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-03 21:22:04]

Dry run: 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-07-03 22:25:34]

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

[commit-bot: I haz the power, 2016-07-03 22:25:36]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[alex clarke (OOO till 29th), 2016-07-04 09:18:30]

Description was changed from

==========
Add commands to manage tabs and contexts to Browser Domain

BUG=
==========

to

==========
Add commands to manage tabs and contexts to Browser Domain

BUG=546953
==========

[alex clarke (OOO till 29th), 2016-07-04 14:23:07]

The CQ bit was checked by alexclarke@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-04 14:23:14]

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

[alex clarke (OOO till 29th), 2016-07-04 14:55:24]

The CQ bit was checked by alexclarke@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-04 14:55:30]

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

[alex clarke (OOO till 29th), 2016-07-04 14:58:34]

Description was changed from

==========
Add commands to manage tabs and contexts to Browser Domain

BUG=546953
==========

to

==========
Add commands to manage tabs and contexts to Browser Domain

Browser.newPage and Browser.closePage are usable in both normal chrome
and in headless chrome.  Browser.newContext and Browser.closeContext
are implemented in headless chrome only.

Note Browser.newPage has a number of headless chrome only
parameters: width, height, contextId



BUG=546953
==========

[alex clarke (OOO till 29th), 2016-07-04 14:59:00]

alexclarke@chromium.org changed reviewers:
+ skyostil@chromium.org

[alex clarke (OOO till 29th), 2016-07-04 14:59:00]

PTAL

[Sami, 2016-07-04 15:28:54]

Nice, this is going to be useful.

https://codereview.chromium.org/2119063002/diff/40001/components/devtools_dis...
File components/devtools_discovery/devtools_discovery_manager.h (right):

https://codereview.chromium.org/2119063002/diff/40001/components/devtools_dis...
components/devtools_discovery/devtools_discovery_manager.h:44:
base::DictionaryValue* MaybeHandleNewPageCommand(
unique_ptr<base::DictionaryValue> to pass ownership?

https://codereview.chromium.org/2119063002/diff/40001/headless/public/headles...
File headless/public/headless_browser.h (right):

https://codereview.chromium.org/2119063002/diff/40001/headless/public/headles...
headless/public/headless_browser.h:41: class HEADLESS_EXPORT Observer {
Could we instead add a getter that turns a target id into a HeadlessWebContents
object? If we want these to be events, we could probably add them to the
devtools browser domain.

https://codereview.chromium.org/2119063002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/browser_protocol.json (right):

https://codereview.chromium.org/2119063002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/browser_protocol.json:4259: "name":
"newContext",
I'll leave this up to Pavel & co, but "newBrowserContext" might be slightly more
descriptive.

[commit-bot: I haz the power, 2016-07-04 16:15:24]

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

[commit-bot: I haz the power, 2016-07-04 16:15:25]

Dry run: This issue passed the CQ dry run.

[alex clarke (OOO till 29th), 2016-07-04 16:33:35]

The CQ bit was checked by alexclarke@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-04 16:33:44]

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

[alex clarke (OOO till 29th), 2016-07-04 16:33:58]

alexclarke@chromium.org changed reviewers:
+ pfeldman@chromium.org

[alex clarke (OOO till 29th), 2016-07-04 16:33:59]

+Pavel PTAL

https://codereview.chromium.org/2119063002/diff/40001/components/devtools_dis...
File components/devtools_discovery/devtools_discovery_manager.h (right):

https://codereview.chromium.org/2119063002/diff/40001/components/devtools_dis...
components/devtools_discovery/devtools_discovery_manager.h:44:
base::DictionaryValue* MaybeHandleNewPageCommand(
On 2016/07/04 15:28:54, Sami wrote:
> unique_ptr<base::DictionaryValue> to pass ownership?

Done.

https://codereview.chromium.org/2119063002/diff/40001/headless/public/headles...
File headless/public/headless_browser.h (right):

https://codereview.chromium.org/2119063002/diff/40001/headless/public/headles...
headless/public/headless_browser.h:41: class HEADLESS_EXPORT Observer {
On 2016/07/04 15:28:54, Sami wrote:
> Could we instead add a getter that turns a target id into a
HeadlessWebContents
> object? If we want these to be events, we could probably add them to the
> devtools browser domain.

I only really added it for testing when I had a different way of closing the
page which made it async. I think we can live without it now.

https://codereview.chromium.org/2119063002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/browser_protocol.json (right):

https://codereview.chromium.org/2119063002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/browser_protocol.json:4259: "name":
"newContext",
On 2016/07/04 15:28:54, Sami wrote:
> I'll leave this up to Pavel & co, but "newBrowserContext" might be slightly
more
> descriptive.

Done.

[commit-bot: I haz the power, 2016-07-04 18:15:46]

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

[commit-bot: I haz the power, 2016-07-04 18:15:47]

Dry run: This issue passed the CQ dry run.

[Sami, 2016-07-05 09:41:36]

https://codereview.chromium.org/2119063002/diff/60001/headless/lib/browser/he...
File headless/lib/browser/headless_devtools_manager_delegate.cc (right):

https://codereview.chromium.org/2119063002/diff/60001/headless/lib/browser/he...
headless/lib/browser/headless_devtools_manager_delegate.cc:62: int height = 800;
600?

https://codereview.chromium.org/2119063002/diff/60001/headless/lib/browser/he...
File headless/lib/browser/headless_devtools_manager_delegate.h (right):

https://codereview.chromium.org/2119063002/diff/60001/headless/lib/browser/he...
headless/lib/browser/headless_devtools_manager_delegate.h:36:
std::unique_ptr<base::Value> newPage(const base::DictionaryValue* params);
nit: NewPage etc.

https://codereview.chromium.org/2119063002/diff/60001/headless/lib/browser/he...
headless/lib/browser/headless_devtools_manager_delegate.h:46:
std::map<std::string, HeadlessWebContentsImpl*> web_contents_map_;
Should we move this map to |browser_| (replacing the one already there)? That
way these commands would work with all pages -- not just ones created through
here (e.g., there's a Browser.getTargets command which can list all existing
targets.)

[alex clarke (OOO till 29th), 2016-07-05 11:53:50]

The CQ bit was checked by alexclarke@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-05 11:53:57]

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

[alex clarke (OOO till 29th), 2016-07-05 11:54:00]

PTAL

https://codereview.chromium.org/2119063002/diff/60001/headless/lib/browser/he...
File headless/lib/browser/headless_devtools_manager_delegate.cc (right):

https://codereview.chromium.org/2119063002/diff/60001/headless/lib/browser/he...
headless/lib/browser/headless_devtools_manager_delegate.cc:62: int height = 800;
On 2016/07/05 09:41:35, Sami wrote:
> 600?

Done.

https://codereview.chromium.org/2119063002/diff/60001/headless/lib/browser/he...
File headless/lib/browser/headless_devtools_manager_delegate.h (right):

https://codereview.chromium.org/2119063002/diff/60001/headless/lib/browser/he...
headless/lib/browser/headless_devtools_manager_delegate.h:36:
std::unique_ptr<base::Value> newPage(const base::DictionaryValue* params);
On 2016/07/05 09:41:35, Sami wrote:
> nit: NewPage etc.

Done.

https://codereview.chromium.org/2119063002/diff/60001/headless/lib/browser/he...
headless/lib/browser/headless_devtools_manager_delegate.h:46:
std::map<std::string, HeadlessWebContentsImpl*> web_contents_map_;
On 2016/07/05 09:41:35, Sami wrote:
> Should we move this map to |browser_| (replacing the one already there)? That
> way these commands would work with all pages -- not just ones created through
> here (e.g., there's a Browser.getTargets command which can list all existing
> targets.)

Done.

[commit-bot: I haz the power, 2016-07-05 13:07:48]

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

[commit-bot: I haz the power, 2016-07-05 13:07:49]

Dry run: This issue passed the CQ dry run.

[Sami, 2016-07-06 13:47:44]

Thanks, lgtm!

[pfeldman, 2016-07-06 17:38:08]

pfeldman@chromium.org changed reviewers:
+ dgozman@chromium.org

[pfeldman, 2016-07-06 17:38:09]

+dgozman

overal looks good, a couple of naming nits.

https://codereview.chromium.org/2119063002/diff/80001/chrome/browser/devtools...
File chrome/browser/devtools/chrome_devtools_manager_delegate.cc (right):

https://codereview.chromium.org/2119063002/diff/80001/chrome/browser/devtools...
chrome/browser/devtools/chrome_devtools_manager_delegate.cc:51:
DevToolsDiscoveryManager::GetInstance()->MaybeHandleNewPageCommand(
It seems we are skipping the Maybe prefix in other places (like below).

https://codereview.chromium.org/2119063002/diff/80001/content/browser/devtool...
File content/browser/devtools/protocol/browser_handler.h (right):

https://codereview.chromium.org/2119063002/diff/80001/content/browser/devtool...
content/browser/devtools/protocol/browser_handler.h:38: Response
CloseBrowserContext(const std::string& contextId, bool* out_success);
context_id

https://codereview.chromium.org/2119063002/diff/80001/content/browser/devtool...
content/browser/devtools/protocol/browser_handler.h:39: Response NewPage(const
std::string& in_initialUrl,
ditto

https://codereview.chromium.org/2119063002/diff/80001/content/browser/devtool...
content/browser/devtools/protocol/browser_handler.h:40: const int* in_width,
Providing a subset of the device metrics data seems suboptimal - what if we want
to consistently create new pages for mobile.

https://codereview.chromium.org/2119063002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/browser_protocol.json (right):

https://codereview.chromium.org/2119063002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/browser_protocol.json:4259: "name":
"newBrowserContext",
createBrowserContext

https://codereview.chromium.org/2119063002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/browser_protocol.json:4267: "name":
"closeBrowserContext",
disposeBrowserContext?

https://codereview.chromium.org/2119063002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/browser_protocol.json:4278: "name":
"newPage",
createPage

https://codereview.chromium.org/2119063002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/browser_protocol.json:4282: { "name":
"width", "type": "integer", "description": "Window width (headless chrome
only).", "optional": true },
Still hesitant about these - my worry is that they are not sufficient and
emulation will anyways be used.

[dgozman, 2016-07-06 18:33:31]

It's unfortunate that we have to manually plumb from DevToolsManagerDelegate to
devtools_discovery, but I don't have any easy solution right now. Overall code
looks good.

https://codereview.chromium.org/2119063002/diff/80001/components/devtools_dis...
File components/devtools_discovery/devtools_discovery_manager.cc (right):

https://codereview.chromium.org/2119063002/diff/80001/components/devtools_dis...
components/devtools_discovery/devtools_discovery_manager.cc:78:
CreateNew(GURL(initial_url));
CreateNew may return nullptr.

https://codereview.chromium.org/2119063002/diff/80001/content/browser/devtool...
File content/browser/devtools/protocol/browser_handler.cc (right):

https://codereview.chromium.org/2119063002/diff/80001/content/browser/devtool...
content/browser/devtools/protocol/browser_handler.cc:47: // For layering reasons
this neends to be handled by
typo: neends

https://codereview.chromium.org/2119063002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/browser_protocol.json (right):

https://codereview.chromium.org/2119063002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/browser_protocol.json:4292: "name":
"closePage",
I think this works for any target, isn't it? Let's make it closeTarget.
Same goes for newPage, let's call it createTarget?

[alex clarke (OOO till 29th), 2016-07-06 20:24:54]

The CQ bit was checked by alexclarke@chromium.org to run a CQ dry run

[alex clarke (OOO till 29th), 2016-07-06 20:25:07]

PTAL

https://codereview.chromium.org/2119063002/diff/80001/chrome/browser/devtools...
File chrome/browser/devtools/chrome_devtools_manager_delegate.cc (right):

https://codereview.chromium.org/2119063002/diff/80001/chrome/browser/devtools...
chrome/browser/devtools/chrome_devtools_manager_delegate.cc:51:
DevToolsDiscoveryManager::GetInstance()->MaybeHandleNewPageCommand(
On 2016/07/06 17:38:09, pfeldman wrote:
> It seems we are skipping the Maybe prefix in other places (like below).

Done.

https://codereview.chromium.org/2119063002/diff/80001/components/devtools_dis...
File components/devtools_discovery/devtools_discovery_manager.cc (right):

https://codereview.chromium.org/2119063002/diff/80001/components/devtools_dis...
components/devtools_discovery/devtools_discovery_manager.cc:78:
CreateNew(GURL(initial_url));
On 2016/07/06 18:33:30, dgozman wrote:
> CreateNew may return nullptr.

Good point, thanks.

https://codereview.chromium.org/2119063002/diff/80001/content/browser/devtool...
File content/browser/devtools/protocol/browser_handler.cc (right):

https://codereview.chromium.org/2119063002/diff/80001/content/browser/devtool...
content/browser/devtools/protocol/browser_handler.cc:47: // For layering reasons
this neends to be handled by
On 2016/07/06 18:33:30, dgozman wrote:
> typo: neends

Done.

https://codereview.chromium.org/2119063002/diff/80001/content/browser/devtool...
File content/browser/devtools/protocol/browser_handler.h (right):

https://codereview.chromium.org/2119063002/diff/80001/content/browser/devtool...
content/browser/devtools/protocol/browser_handler.h:38: Response
CloseBrowserContext(const std::string& contextId, bool* out_success);
On 2016/07/06 17:38:09, pfeldman wrote:
> context_id

Done.

https://codereview.chromium.org/2119063002/diff/80001/content/browser/devtool...
content/browser/devtools/protocol/browser_handler.h:39: Response NewPage(const
std::string& in_initialUrl,
On 2016/07/06 17:38:09, pfeldman wrote:
> ditto

Done.

https://codereview.chromium.org/2119063002/diff/80001/content/browser/devtool...
content/browser/devtools/protocol/browser_handler.h:40: const int* in_width,
On 2016/07/06 17:38:09, pfeldman wrote:
> Providing a subset of the device metrics data seems suboptimal - what if we
want
> to consistently create new pages for mobile.

I'm not quite sure what you are suggesting.  I note the params for
setDeviceMetricsOverride do you want those here?

https://codereview.chromium.org/2119063002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/browser_protocol.json (right):

https://codereview.chromium.org/2119063002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/browser_protocol.json:4259: "name":
"newBrowserContext",
On 2016/07/06 17:38:09, pfeldman wrote:
> createBrowserContext

Done.

https://codereview.chromium.org/2119063002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/browser_protocol.json:4267: "name":
"closeBrowserContext",
On 2016/07/06 17:38:09, pfeldman wrote:
> disposeBrowserContext?

Done.

https://codereview.chromium.org/2119063002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/browser_protocol.json:4278: "name":
"newPage",
On 2016/07/06 17:38:09, pfeldman wrote:
> createPage

Dimitri suggested createTarget, does that work for you?

https://codereview.chromium.org/2119063002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/browser_protocol.json:4282: { "name":
"width", "type": "integer", "description": "Window width (headless chrome
only).", "optional": true },
On 2016/07/06 17:38:09, pfeldman wrote:
> Still hesitant about these - my worry is that they are not sufficient
For regular chrome?

> and emulation will anyways be used.

https://codereview.chromium.org/2119063002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/browser_protocol.json:4292: "name":
"closePage",
On 2016/07/06 18:33:30, dgozman wrote:
> I think this works for any target, isn't it? Let's make it closeTarget.
> Same goes for newPage, let's call it createTarget?

Done.

[commit-bot: I haz the power, 2016-07-06 20:25:21]

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

[pfeldman, 2016-07-06 20:47:48]

> I'm not quite sure what you are suggesting.  I note the params for
setDeviceMetricsOverride do you want those here?

I was rather suggesting that in reality, createTarget would be always followed
by setDeviceMetricsOverride, hence width and height did not make much sense
here.

https://codereview.chromium.org/2119063002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/inspector/browser_protocol.json (right):

https://codereview.chromium.org/2119063002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/browser_protocol.json:4278: "name":
"newTarget",
createTarget

[alex clarke (OOO till 29th), 2016-07-06 21:02:57]

On 2016/07/06 20:47:48, pfeldman wrote:
> > I'm not quite sure what you are suggesting.  I note the params for
> setDeviceMetricsOverride do you want those here?
> 
> I was rather suggesting that in reality, createTarget would be always followed
> by setDeviceMetricsOverride, hence width and height did not make much sense
> here.

Oh I see what you mean.  Well for headless browser we need to know how big the
backing buffer we need to allocate should be, so in that context it's needed.

> 
>
https://codereview.chromium.org/2119063002/diff/100001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/inspector/browser_protocol.json (right):
> 
>
https://codereview.chromium.org/2119063002/diff/100001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/inspector/browser_protocol.json:4278: "name":
> "newTarget",
> createTarget

[alex clarke (OOO till 29th), 2016-07-06 21:03:20]

PTAL

https://codereview.chromium.org/2119063002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/inspector/browser_protocol.json (right):

https://codereview.chromium.org/2119063002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/browser_protocol.json:4278: "name":
"newTarget",
On 2016/07/06 20:47:48, pfeldman wrote:
> createTarget

Done.

[alex clarke (OOO till 29th), 2016-07-06 21:03:29]

The CQ bit was checked by alexclarke@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-06 21:04:14]

Dry run: 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-07-07 00:25:41]

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

[commit-bot: I haz the power, 2016-07-07 00:25:42]

Dry run: This issue passed the CQ dry run.

[pfeldman, 2016-07-07 00:41:18]

lgtm

[alex clarke (OOO till 29th), 2016-07-07 08:03:34]

The CQ bit was checked by alexclarke@chromium.org

[alex clarke (OOO till 29th), 2016-07-07 08:03:34]

The patchset sent to the CQ was uploaded after l-g-t-m from
skyostil@chromium.org
Link to the patchset: https://codereview.chromium.org/2119063002/#ps120001
(title: "Fix name")

[commit-bot: I haz the power, 2016-07-07 08:03:47]

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-07-07 08:08:10]

Description was changed from

==========
Add commands to manage tabs and contexts to Browser Domain

Browser.newPage and Browser.closePage are usable in both normal chrome
and in headless chrome.  Browser.newContext and Browser.closeContext
are implemented in headless chrome only.

Note Browser.newPage has a number of headless chrome only
parameters: width, height, contextId



BUG=546953
==========

to

==========
Add commands to manage tabs and contexts to Browser Domain

Browser.newPage and Browser.closePage are usable in both normal chrome
and in headless chrome.  Browser.newContext and Browser.closeContext
are implemented in headless chrome only.

Note Browser.newPage has a number of headless chrome only
parameters: width, height, contextId



BUG=546953
==========

[commit-bot: I haz the power, 2016-07-07 08:08:12]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-07-07 08:09:20]

Description was changed from

==========
Add commands to manage tabs and contexts to Browser Domain

Browser.newPage and Browser.closePage are usable in both normal chrome
and in headless chrome.  Browser.newContext and Browser.closeContext
are implemented in headless chrome only.

Note Browser.newPage has a number of headless chrome only
parameters: width, height, contextId



BUG=546953
==========

to

==========
Add commands to manage tabs and contexts to Browser Domain

Browser.newPage and Browser.closePage are usable in both normal chrome
and in headless chrome.  Browser.newContext and Browser.closeContext
are implemented in headless chrome only.

Note Browser.newPage has a number of headless chrome only
parameters: width, height, contextId

BUG=546953

Committed: https://crrev.com/72564fc55f6c9a186fca1055004c1c892d68026d
Cr-Commit-Position: refs/heads/master@{#404099}
==========

[commit-bot: I haz the power, 2016-07-07 08:09:22]

Patchset 7 (id:??) landed as
https://crrev.com/72564fc55f6c9a186fca1055004c1c892d68026d
Cr-Commit-Position: refs/heads/master@{#404099}

[Lei Zhang, 2017-03-29 01:09:10]

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

[Lei Zhang, 2017-03-29 01:09:11]

https://codereview.chromium.org/2119063002/diff/120001/headless/lib/browser/h...
File headless/lib/browser/headless_devtools_manager_delegate.h (right):

https://codereview.chromium.org/2119063002/diff/120001/headless/lib/browser/h...
headless/lib/browser/headless_devtools_manager_delegate.h:48: using
CommandMemberFnPtr = std::unique_ptr<base::Value> (
I didn't see any discussion, so I'm curious why this uses function pointers,
instead of base::Callbacks.

[alex clarke (OOO till 29th), 2017-03-29 09:14:53]

https://codereview.chromium.org/2119063002/diff/120001/headless/lib/browser/h...
File headless/lib/browser/headless_devtools_manager_delegate.h (right):

https://codereview.chromium.org/2119063002/diff/120001/headless/lib/browser/h...
headless/lib/browser/headless_devtools_manager_delegate.h:48: using
CommandMemberFnPtr = std::unique_ptr<base::Value> (
On 2017/03/29 01:09:11, Lei Zhang wrote:
> I didn't see any discussion, so I'm curious why this uses function pointers,
> instead of base::Callbacks.

I'm probably missing some context, I suppose we could have used base::Callbacks,
does this matter?

[Lei Zhang, 2017-03-29 09:40:44]

On 2017/03/29 09:14:53, alex clarke wrote:
>
https://codereview.chromium.org/2119063002/diff/120001/headless/lib/browser/h...
> File headless/lib/browser/headless_devtools_manager_delegate.h (right):
> 
>
https://codereview.chromium.org/2119063002/diff/120001/headless/lib/browser/h...
> headless/lib/browser/headless_devtools_manager_delegate.h:48: using
> CommandMemberFnPtr = std::unique_ptr<base::Value> (
> On 2017/03/29 01:09:11, Lei Zhang wrote:
> > I didn't see any discussion, so I'm curious why this uses function pointers,
> > instead of base::Callbacks.
> 
> I'm probably missing some context, I suppose we could have used
base::Callbacks,
> does this matter?

Context: I'm looking at https://codereview.chromium.org/2780433002/ and it's
copying the function pointer pattern from the code in this CL.

I'm not saying to never used function pointers, but base::Callbacks is the most
commonly used and well understood way of doing callbacks in the code base.
There's lots of well-maintained support code in base/bind_helpers.h to work with
base::Callbacks that you don't get with plain function pointers. So I'm
wondering why not use base::Callbacks? What makes function pointers better?

[alex clarke (OOO till 29th), 2017-03-29 09:51:13]

On 2017/03/29 09:40:44, Lei Zhang wrote:
> On 2017/03/29 09:14:53, alex clarke wrote:
> >
>
https://codereview.chromium.org/2119063002/diff/120001/headless/lib/browser/h...
> > File headless/lib/browser/headless_devtools_manager_delegate.h (right):
> > 
> >
>
https://codereview.chromium.org/2119063002/diff/120001/headless/lib/browser/h...
> > headless/lib/browser/headless_devtools_manager_delegate.h:48: using
> > CommandMemberFnPtr = std::unique_ptr<base::Value> (
> > On 2017/03/29 01:09:11, Lei Zhang wrote:
> > > I didn't see any discussion, so I'm curious why this uses function
pointers,
> > > instead of base::Callbacks.
> > 
> > I'm probably missing some context, I suppose we could have used
> base::Callbacks,
> > does this matter?
> 
> Context: I'm looking at https://codereview.chromium.org/2780433002/ and it's
> copying the function pointer pattern from the code in this CL.
> 
> I'm not saying to never used function pointers, but base::Callbacks is the
most
> commonly used and well understood way of doing callbacks in the code base.
> There's lots of well-maintained support code in base/bind_helpers.h to work
with
> base::Callbacks that you don't get with plain function pointers. So I'm
> wondering why not use base::Callbacks? What makes function pointers better?

I don't think it makes much difference, but if you want to change it in that
patch then please go ahead :)

[Lei Zhang, 2017-03-29 10:04:15]

On 2017/03/29 09:51:13, alex clarke wrote:
> I don't think it makes much difference, but if you want to change it in that
> patch then please go ahead :)

Thanks for the feedback. Maybe in a separate cleanup CL. If it doesn't matter
either way, then let's go for more consistency in the code base.