Cleanup LocalDiscoveryUIHandler.

Cleanup LocalDiscoveryUIHandler.

- PDF plugin is always available.
- CloudPrintProxyServiceFactory::GetForProfile() never returns nullptr.
- Various nits.

Committed: https://crrev.com/b53f23dbb4d0cafc39b26cd9daa230d4722f0989
Cr-Commit-Position: refs/heads/master@{#388954}

[Lei Zhang, 2016-04-16 01:10:02]

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

[Lei Zhang, 2016-04-16 01:10:59]

Description was changed from

==========
Cleanup LocalDiscoveryUIHandler.

- PDF plugin is always available.
- CloudPrintProxyServiceFactory::GetForProfile() never ruturns nullptr.
- Various nits.
==========

to

==========
Cleanup LocalDiscoveryUIHandler.

- PDF plugin is always available.
- CloudPrintProxyServiceFactory::GetForProfile() never returns nullptr.
- Various nits.
==========

[Lei Zhang, 2016-04-16 01:11:39]

"CloudPrintProxyServiceFactory::GetForProfile() never returns nullptr" ...
assuming it's got a valid Profile, which should be the case.

[Vitaly Buka (NO REVIEWS), 2016-04-16 01:33:52]

https://codereview.chromium.org/1898503002/diff/1/chrome/browser/extensions/a...
File
chrome/browser/extensions/api/cloud_print_private/cloud_print_private_api.cc
(left):

https://codereview.chromium.org/1898503002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/cloud_print_private/cloud_print_private_api.cc:54:
return false;
I guess this could be NULL for for incognito.

https://codereview.chromium.org/1898503002/diff/1/chrome/browser/local_discov...
File chrome/browser/local_discovery/service_discovery_device_lister.cc (right):

https://codereview.chromium.org/1898503002/diff/1/chrome/browser/local_discov...
chrome/browser/local_discovery/service_discovery_device_lister.cc:67:
linked_ptr<ServiceResolver>(NULL)));
scoped_ptrs didn't work well with std containers
maybe linked_ptr can be replaced with unique_ptr

https://codereview.chromium.org/1898503002/diff/1/chrome/browser/ui/webui/loc...
File chrome/browser/ui/webui/local_discovery/local_discovery_ui_handler.h
(left):

https://codereview.chromium.org/1898503002/diff/1/chrome/browser/ui/webui/loc...
chrome/browser/ui/webui/local_discovery/local_discovery_ui_handler.h:200: bool
cloud_print_connector_ui_enabled_;
you are replacing this with def?

[Lei Zhang, 2016-04-16 01:53:20]

https://codereview.chromium.org/1898503002/diff/1/chrome/browser/extensions/a...
File
chrome/browser/extensions/api/cloud_print_private/cloud_print_private_api.cc
(left):

https://codereview.chromium.org/1898503002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/cloud_print_private/cloud_print_private_api.cc:54:
return false;
On 2016/04/16 01:33:52, Vitaly Buka wrote:
> I guess this could be NULL for for incognito.

Ok, I will look into this. How is this API used anyway?

At least I passed all the existing tests. Should I add another API test and try
to use it in incognito?

At least my thought right now is:

- GetProfile() relies on UIThreadExtensionFunction's context.
- UIThreadExtensionFunction's context starts out NULL, but 
ExtensionFunctionDispatcher calls
UIThreadExtensionFunction::set_browser_context() before running any functions.
- ExtensionFunctionDispatcher's context comes from ExtensionWebContentsObserver.
- ExtensionFunctionDispatcher's context comes from a WebContents.
- WebContents should have a valid context?

https://codereview.chromium.org/1898503002/diff/1/chrome/browser/local_discov...
File chrome/browser/local_discovery/service_discovery_device_lister.cc (right):

https://codereview.chromium.org/1898503002/diff/1/chrome/browser/local_discov...
chrome/browser/local_discovery/service_discovery_device_lister.cc:67:
linked_ptr<ServiceResolver>(NULL)));
On 2016/04/16 01:33:52, Vitaly Buka wrote:
> scoped_ptrs didn't work well with std containers
> maybe linked_ptr can be replaced with unique_ptr

Done.

https://codereview.chromium.org/1898503002/diff/1/chrome/browser/ui/webui/loc...
File chrome/browser/ui/webui/local_discovery/local_discovery_ui_handler.h
(left):

https://codereview.chromium.org/1898503002/diff/1/chrome/browser/ui/webui/loc...
chrome/browser/ui/webui/local_discovery/local_discovery_ui_handler.h:200: bool
cloud_print_connector_ui_enabled_;
On 2016/04/16 01:33:52, Vitaly Buka wrote:
> you are replacing this with def?

I'm assuming this is always true now.

[Vitaly Buka (NO REVIEWS), 2016-04-16 02:17:56]

lgtm if you either revert if(!service) or check incognito case.

https://codereview.chromium.org/1898503002/diff/1/chrome/browser/extensions/a...
File
chrome/browser/extensions/api/cloud_print_private/cloud_print_private_api.cc
(left):

https://codereview.chromium.org/1898503002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/cloud_print_private/cloud_print_private_api.cc:54:
return false;
On 2016/04/16 01:53:19, Lei Zhang wrote:
> On 2016/04/16 01:33:52, Vitaly Buka wrote:
> > I guess this could be NULL for for incognito.
> 
> Ok, I will look into this. How is this API used anyway?
> 
> At least I passed all the existing tests. Should I add another API test and
try
> to use it in incognito?

Test would be nice.

This API used only by
https://www.google.com/cloudprint/enable_chrome_connector/enable.html.
chrome://devices only redirects to that page.

Try to navigate to
https://www.google.com/cloudprint/enable_chrome_connector/enable.html?proxy=N...

In normal tab you should see printers list. and "add" button.
Incognito should not crash. Now it displays empty list of printers and no "add"
button.
I suspect it will crash without if (!service).

If you know how disable API for incognito, then you probably can remove if
(!service).

> 
> At least my thought right now is:
> 
> - GetProfile() relies on UIThreadExtensionFunction's context.
> - UIThreadExtensionFunction's context starts out NULL, but 
> ExtensionFunctionDispatcher calls
> UIThreadExtensionFunction::set_browser_context() before running any functions.
> - ExtensionFunctionDispatcher's context comes from
ExtensionWebContentsObserver.
> - ExtensionFunctionDispatcher's context comes from a WebContents.
> - WebContents should have a valid context?

https://codereview.chromium.org/1898503002/diff/1/chrome/browser/ui/webui/loc...
File chrome/browser/ui/webui/local_discovery/local_discovery_ui_handler.h
(left):

https://codereview.chromium.org/1898503002/diff/1/chrome/browser/ui/webui/loc...
chrome/browser/ui/webui/local_discovery/local_discovery_ui_handler.h:200: bool
cloud_print_connector_ui_enabled_;
On 2016/04/16 01:53:19, Lei Zhang wrote:
> On 2016/04/16 01:33:52, Vitaly Buka wrote:
> > you are replacing this with def?
> 
> I'm assuming this is always true now.

sgtm

[Lei Zhang, 2016-04-21 01:08:41]

PTAL at PS2 -> PS6 diff.

I added a test and indeed CloudPrintProxyServiceFactory::GetForProfile() returns
nullptr for incognito mode. So I added the checks back in for the extension API.

With LocalDiscoveryUIHandler OTOH, if I try to navigate to chrome://devices in
an incognito window, it opens in the real profile instead. So the
CloudPrintProxyServiceFactory::GetForProfile() call there should always work.

https://codereview.chromium.org/1898503002/diff/100001/chrome/browser/extensi...
File
chrome/browser/extensions/api/cloud_print_private/cloud_print_private_api.cc
(right):

https://codereview.chromium.org/1898503002/diff/100001/chrome/browser/extensi...
chrome/browser/extensions/api/cloud_print_private/cloud_print_private_api.cc:20:
const char kErrorIncognito[] = "Cannot access in incognito mode";
Needed to add an error message to make chrome.test.assertLastError() work in the
new incognito test case.

[Vitaly Buka (NO REVIEWS), 2016-04-21 20:09:45]

lgtm

Thank you for adding the test.

https://codereview.chromium.org/1898503002/diff/100001/chrome/browser/extensi...
File
chrome/browser/extensions/api/cloud_print_private/cloud_print_private_apitest.cc
(right):

https://codereview.chromium.org/1898503002/diff/100001/chrome/browser/extensi...
chrome/browser/extensions/api/cloud_print_private/cloud_print_private_apitest.cc:64:
CloudPrintTestsDelegateMock() {}
why not just use the default destructor?

[Lei Zhang, 2016-04-21 20:50:22]

https://codereview.chromium.org/1898503002/diff/100001/chrome/browser/extensi...
File
chrome/browser/extensions/api/cloud_print_private/cloud_print_private_apitest.cc
(right):

https://codereview.chromium.org/1898503002/diff/100001/chrome/browser/extensi...
chrome/browser/extensions/api/cloud_print_private/cloud_print_private_apitest.cc:64:
CloudPrintTestsDelegateMock() {}
On 2016/04/21 20:09:45, Vitaly Buka wrote:
> why not just use the default destructor?

Did you mean with " = default;" or leave it off?

I was playing around with making the test work and at one point added other
overrides and wanted to explicitly follow the class with virtual methods should
have a virtual destruction rule.

[Vitaly Buka (NO REVIEWS), 2016-04-21 22:03:45]

On 2016/04/21 20:50:22, Lei Zhang wrote:
>
https://codereview.chromium.org/1898503002/diff/100001/chrome/browser/extensi...
> File
>
chrome/browser/extensions/api/cloud_print_private/cloud_print_private_apitest.cc
> (right):
> 
>
https://codereview.chromium.org/1898503002/diff/100001/chrome/browser/extensi...
>
chrome/browser/extensions/api/cloud_print_private/cloud_print_private_apitest.cc:64:
> CloudPrintTestsDelegateMock() {}
> On 2016/04/21 20:09:45, Vitaly Buka wrote:
> > why not just use the default destructor?
> 
> Did you mean with " = default;" or leave it off?

Just leave it off, I guess chrome does not use = default.

> 
> I was playing around with making the test work and at one point added other
> overrides and wanted to explicitly follow the class with virtual methods
should
> have a virtual destruction rule.

sgtm

[Vitaly Buka (NO REVIEWS), 2016-04-21 22:05:10]

On 2016/04/21 22:03:45, Vitaly Buka wrote:
> On 2016/04/21 20:50:22, Lei Zhang wrote:
> >
>
https://codereview.chromium.org/1898503002/diff/100001/chrome/browser/extensi...
> > File
> >
>
chrome/browser/extensions/api/cloud_print_private/cloud_print_private_apitest.cc
> > (right):
> > 
> >
>
https://codereview.chromium.org/1898503002/diff/100001/chrome/browser/extensi...
> >
>
chrome/browser/extensions/api/cloud_print_private/cloud_print_private_apitest.cc:64:
> > CloudPrintTestsDelegateMock() {}
> > On 2016/04/21 20:09:45, Vitaly Buka wrote:
> > > why not just use the default destructor?
> > 
> > Did you mean with " = default;" or leave it off?
> 
> Just leave it off, I guess chrome does not use = default.

Actually it's allowed.
https://chromium-cpp.appspot.com/

[Lei Zhang, 2016-04-21 22:14:55]

On 2016/04/21 22:03:45, Vitaly Buka wrote:
> Just leave it off, I guess chrome does not use = default.

Done.

[Lei Zhang, 2016-04-21 22:15:33]

The CQ bit was checked by thestig@chromium.org

[Lei Zhang, 2016-04-21 22:15:34]

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

[commit-bot: I haz the power, 2016-04-21 22:15:58]

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

[commit-bot: I haz the power, 2016-04-21 22:57:16]

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

[commit-bot: I haz the power, 2016-04-21 22:57:17]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)

[Lei Zhang, 2016-04-21 22:58:00]

The CQ bit was checked by thestig@chromium.org

[commit-bot: I haz the power, 2016-04-21 22:58:17]

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

[commit-bot: I haz the power, 2016-04-21 23:45:15]

Description was changed from

==========
Cleanup LocalDiscoveryUIHandler.

- PDF plugin is always available.
- CloudPrintProxyServiceFactory::GetForProfile() never returns nullptr.
- Various nits.
==========

to

==========
Cleanup LocalDiscoveryUIHandler.

- PDF plugin is always available.
- CloudPrintProxyServiceFactory::GetForProfile() never returns nullptr.
- Various nits.
==========

[commit-bot: I haz the power, 2016-04-21 23:45:16]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-04-22 19:41:40]

Description was changed from

==========
Cleanup LocalDiscoveryUIHandler.

- PDF plugin is always available.
- CloudPrintProxyServiceFactory::GetForProfile() never returns nullptr.
- Various nits.
==========

to

==========
Cleanup LocalDiscoveryUIHandler.

- PDF plugin is always available.
- CloudPrintProxyServiceFactory::GetForProfile() never returns nullptr.
- Various nits.

Committed: https://crrev.com/b53f23dbb4d0cafc39b26cd9daa230d4722f0989
Cr-Commit-Position: refs/heads/master@{#388954}
==========

[commit-bot: I haz the power, 2016-04-22 19:41:42]

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