[MD settings] add getSiteDetails to site settings browser proxy

[MD settings] add getSiteDetails to site settings browser proxy

This CL is a step toward deep linking to site details in the site
settings. This CL adds the c++ handler backend and does show the
exceptions in the UI. The page title is not updated with the subject URL,
but that is not a regression and will be addressed in a future CL.

BUG=635874
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

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

[dschuyler, 2016-09-14 22:50:22]

Description was changed from

==========
[MD settings] add getSiteDetails to site settings browser proxy

This CL is a step toward deep linking to site details in the site
settings. This CL adds the c++ handler backend and does show the
exceptions in the UI. The page title is not updated with the subject URL,
but that is not a regression and will be addressed in a future CL.

BUG=635874
==========

to

==========
[MD settings] add getSiteDetails to site settings browser proxy

This CL is a step toward deep linking to site details in the site
settings. This CL adds the c++ handler backend and does show the
exceptions in the UI. The page title is not updated with the subject URL,
but that is not a regression and will be addressed in a future CL.

BUG=635874
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[dschuyler, 2016-09-14 22:50:24]

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

[commit-bot: I haz the power, 2016-09-14 22:50:57]

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

[dschuyler, 2016-09-14 22:56:27]

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

[dschuyler, 2016-09-14 22:56:44]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2016-09-14 22:56:57]

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

[dschuyler, 2016-09-14 22:58:10]

dschuyler@chromium.org changed reviewers:
+ finnur@chromium.org

[commit-bot: I haz the power, 2016-09-14 23:06:27]

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

[commit-bot: I haz the power, 2016-09-14 23:06:28]

Dry run: Try jobs failed on following builders:
  closure_compilation on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)

[Finnur, 2016-09-15 12:51:11]

LGTM, with a few comments.

https://codereview.chromium.org/2338163004/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/site_settings/site_details.js (right):

https://codereview.chromium.org/2338163004/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_details.js:72:
this.$.usageApi.fetchUsageTotal(url.hostname);
So, this code runs, for example, on refresh of Site Details because we set the
|site| in line 57. Right?

https://codereview.chromium.org/2338163004/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/site_settings_handler.cc (right):

https://codereview.chromium.org/2338163004/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/site_settings_handler.cc:560: };
Can we somehow enforce (perhaps just with a test) that this does not go out of
sync when a new category is added to the Site Settings UI?

https://codereview.chromium.org/2338163004/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/site_settings_handler.h (right):

https://codereview.chromium.org/2338163004/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/site_settings_handler.h:92: void
HandleGetSiteDetails(const base::ListValue* args);
nit: Comment.

https://codereview.chromium.org/2338163004/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/site_settings_helper.cc (right):

https://codereview.chromium.org/2338163004/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/site_settings_helper.cc:9: #include <utility>
Is this the result of a clang IWYU tool?

Also, string and utility are already included by the .h file.

[dschuyler, 2016-09-15 20:34:22]

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

[commit-bot: I haz the power, 2016-09-15 20:34:42]

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

[dschuyler, 2016-09-15 20:35:28]

https://codereview.chromium.org/2338163004/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/site_settings/site_details.js (right):

https://codereview.chromium.org/2338163004/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_details.js:72:
this.$.usageApi.fetchUsageTotal(url.hostname);
On 2016/09/15 12:51:11, Finnur wrote:
> So, this code runs, for example, on refresh of Site Details because we set the
> |site| in line 57. Right?

It /will/ do so after the originForDisplay is set, which will happen in a later
CL. I'm prepping another CL that will set the page title that will include those
changes.

https://codereview.chromium.org/2338163004/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/site_settings_handler.cc (right):

https://codereview.chromium.org/2338163004/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/site_settings_handler.cc:560: };
On 2016/09/15 12:51:11, Finnur wrote:
> Can we somehow enforce (perhaps just with a test) that this does not go out of
> sync when a new category is added to the Site Settings UI?

I'm not sure on this right off. I've created crbug 647399 to track this.

https://codereview.chromium.org/2338163004/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/site_settings_handler.h (right):

https://codereview.chromium.org/2338163004/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/site_settings_handler.h:92: void
HandleGetSiteDetails(const base::ListValue* args);
On 2016/09/15 12:51:11, Finnur wrote:
> nit: Comment.

Done.

https://codereview.chromium.org/2338163004/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/site_settings_helper.cc (right):

https://codereview.chromium.org/2338163004/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/site_settings_helper.cc:9: #include <utility>
On 2016/09/15 12:51:11, Finnur wrote:
> Is this the result of a clang IWYU tool?

Yes.

> Also, string and utility are already included by the .h file.

Thanks!

[commit-bot: I haz the power, 2016-09-15 21:44:47]

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

[commit-bot: I haz the power, 2016-09-15 21:44:48]

Dry run: This issue passed the CQ dry run.

[dschuyler, 2016-09-15 23:18:30]

The CQ bit was checked by dschuyler@chromium.org

[dschuyler, 2016-09-15 23:18:30]

The patchset sent to the CQ was uploaded after l-g-t-m from finnur@chromium.org
Link to the patchset: https://codereview.chromium.org/2338163004/#ps40001
(title: "review changes")

[commit-bot: I haz the power, 2016-09-15 23:18:58]

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-09-15 23:28:01]

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

[commit-bot: I haz the power, 2016-09-15 23:28:02]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[dschuyler, 2016-09-16 00:00:26]

The CQ bit was checked by dschuyler@chromium.org

[commit-bot: I haz the power, 2016-09-16 00:00:51]

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-09-16 00:09:51]

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

[commit-bot: I haz the power, 2016-09-16 00:09:53]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[dschuyler, 2016-09-16 00:16:26]

dschuyler@chromium.org changed reviewers:
+ dbeam@chromium.org

[dschuyler, 2016-09-16 00:16:27]

dbeam@ for OWNER.

[Dan Beam, 2016-09-16 01:15:45]

lgtm but i think the filter param is weird (but i understand you probably can't
just pass an empty string because that's different?)

[dschuyler, 2016-09-16 01:19:01]

On 2016/09/16 01:15:45, Dan Beam wrote:
> lgtm but i think the filter param is weird (but i understand you probably
can't
> just pass an empty string because that's different?)

I considered and empty string. I could have tested for .empty();
I reasoned that an explicit non-value was better than an actual value
used as a sentinel.

[dschuyler, 2016-09-16 01:19:05]

The CQ bit was checked by dschuyler@chromium.org

[commit-bot: I haz the power, 2016-09-16 01:20:02]

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

[Dan Beam, 2016-09-16 01:23:08]

On 2016/09/16 01:19:01, dschuyler wrote:
> On 2016/09/16 01:15:45, Dan Beam wrote:
> > lgtm but i think the filter param is weird (but i understand you probably
> can't
> > just pass an empty string because that's different?)
> 
> I considered and empty string. I could have tested for .empty();
> I reasoned that an explicit non-value was better than an actual value
> used as a sentinel.

yeah, that's basically what I assume.  it's fine, still lgtm

[commit-bot: I haz the power, 2016-09-16 01:26:33]

Committed patchset #2 (id:40001)

[commit-bot: I haz the power, 2016-09-16 01:28:48]

Description was changed from

==========
[MD settings] add getSiteDetails to site settings browser proxy

This CL is a step toward deep linking to site details in the site
settings. This CL adds the c++ handler backend and does show the
exceptions in the UI. The page title is not updated with the subject URL,
but that is not a regression and will be addressed in a future CL.

BUG=635874
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
[MD settings] add getSiteDetails to site settings browser proxy

This CL is a step toward deep linking to site details in the site
settings. This CL adds the c++ handler backend and does show the
exceptions in the UI. The page title is not updated with the subject URL,
but that is not a regression and will be addressed in a future CL.

BUG=635874
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

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

[commit-bot: I haz the power, 2016-09-16 01:28:50]

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