MD Settings: Amend SiteSettingsHandlerTest.Origins to test "Site Details" code.

MD Settings: Amend SiteSettingsHandlerTest.Origins to test "Site Details" code.

r483614 introduced SiteSettingsHandler::HandleGetOriginPermissions(), which
retrieves the content setting value for a list of ContentSettingsTypes for a
given origin via the PermissionManager. It only adds Javascript test coverage
for the new code, so add test coverage for the C++ side in this patch.

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

Review-Url: https://codereview.chromium.org/2946393002
Cr-Commit-Position: refs/heads/master@{#484465}
Committed: https://chromium.googlesource.com/chromium/src/+/d265af292d6342a1205dc206c7d1f9e87f0a2ce1

[Patti Lor, 2017-06-22 04:37:18]

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

[commit-bot: I haz the power, 2017-06-22 04:37:26]

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, 2017-06-22 06:01:08]

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

[commit-bot: I haz the power, 2017-06-22 06:01:09]

Dry run: This issue passed the CQ dry run.

[Patti Lor, 2017-06-22 08:06:46]

patricialor@chromium.org changed reviewers:
+ raymes@chromium.org

[Patti Lor, 2017-06-22 08:06:47]

Hey Raymes, did you want to take a look at this first? These were the C++ test
changes I made to go with https://codereview.chromium.org/2936003003/.

[raymes, 2017-06-26 04:12:28]

lgtm

https://codereview.chromium.org/2946393002/diff/1/chrome/browser/ui/webui/set...
File chrome/browser/ui/webui/settings/site_settings_handler_unittest.cc (right):

https://codereview.chromium.org/2946393002/diff/1/chrome/browser/ui/webui/set...
chrome/browser/ui/webui/settings/site_settings_handler_unittest.cc:109:
data.arg3()->GetAsDictionary(&exception);
Could you add a comment describing these 2 cases? It's quite subtle - I think
it's basically handling the 2 cases for the exception list and
getCategoryPermissionListForOrigin (unpacking the values of some structure in
the page in each case)?

[Patti Lor, 2017-06-26 06:59:35]

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

[commit-bot: I haz the power, 2017-06-26 06:59:56]

Dry run: CQ is trying da patch.

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

[Patti Lor, 2017-06-26 08:10:53]

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

[Patti Lor, 2017-06-26 08:10:54]

Hi dschuyler, here are some more tests to go with the changes in
https://codereview.chromium.org/2936003003/ - sorry for the noise.

Thanks Raymes!

https://codereview.chromium.org/2946393002/diff/1/chrome/browser/ui/webui/set...
File chrome/browser/ui/webui/settings/site_settings_handler_unittest.cc (right):

https://codereview.chromium.org/2946393002/diff/1/chrome/browser/ui/webui/set...
chrome/browser/ui/webui/settings/site_settings_handler_unittest.cc:109:
data.arg3()->GetAsDictionary(&exception);
On 2017/06/26 04:12:27, raymes wrote:
> Could you add a comment describing these 2 cases? It's quite subtle - I think
> it's basically handling the 2 cases for the exception list and
> getCategoryPermissionListForOrigin (unpacking the values of some structure in
> the page in each case)?

Done.

[commit-bot: I haz the power, 2017-06-26 09:13:37]

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

[commit-bot: I haz the power, 2017-06-26 09:13:38]

Dry run: This issue passed the CQ dry run.

[dschuyler, 2017-06-28 23:57:39]

On 2017/06/26 08:10:54, Patti Lor wrote:
> Hi dschuyler, here are some more tests to go with the changes in
> https://codereview.chromium.org/2936003003/ - sorry for the noise.
> 
> Thanks Raymes!
> 
>
https://codereview.chromium.org/2946393002/diff/1/chrome/browser/ui/webui/set...
> File chrome/browser/ui/webui/settings/site_settings_handler_unittest.cc
(right):
> 
>
https://codereview.chromium.org/2946393002/diff/1/chrome/browser/ui/webui/set...
> chrome/browser/ui/webui/settings/site_settings_handler_unittest.cc:109:
> data.arg3()->GetAsDictionary(&exception);
> On 2017/06/26 04:12:27, raymes wrote:
> > Could you add a comment describing these 2 cases? It's quite subtle - I
think
> > it's basically handling the 2 cases for the exception list and
> > getCategoryPermissionListForOrigin (unpacking the values of some structure
in
> > the page in each case)?
> 
> Done.

I'm guessing this CL will need to change based on changes in
https://codereview.chromium.org/2936003003/ (is that correct?).

[Patti Lor, 2017-06-29 00:59:48]

On 2017/06/28 23:57:39, dschuyler wrote:
> On 2017/06/26 08:10:54, Patti Lor wrote:
> > Hi dschuyler, here are some more tests to go with the changes in
> > https://codereview.chromium.org/2936003003/ - sorry for the noise.
> > 
> > Thanks Raymes!
> > 
> >
>
https://codereview.chromium.org/2946393002/diff/1/chrome/browser/ui/webui/set...
> > File chrome/browser/ui/webui/settings/site_settings_handler_unittest.cc
> (right):
> > 
> >
>
https://codereview.chromium.org/2946393002/diff/1/chrome/browser/ui/webui/set...
> > chrome/browser/ui/webui/settings/site_settings_handler_unittest.cc:109:
> > data.arg3()->GetAsDictionary(&exception);
> > On 2017/06/26 04:12:27, raymes wrote:
> > > Could you add a comment describing these 2 cases? It's quite subtle - I
> think
> > > it's basically handling the 2 cases for the exception list and
> > > getCategoryPermissionListForOrigin (unpacking the values of some structure
> in
> > > the page in each case)?
> > 
> > Done.
> 
> I'm guessing this CL will need to change based on changes in
> https://codereview.chromium.org/2936003003/ (is that correct?).

Yes - sorry, I should have messaged to say this wasn't ready after the changes
in 2936003003, I'll ping this CL again when it's ready.

[Patti Lor, 2017-06-29 01:49:53]

Description was changed from

==========
MD Settings: Amend SiteSettingsHandlerTest.Origins to test "Site Details" code.

https://crrev.com/2936003003 introduced
SiteSettingsHandler::HandleGetCategoryPermissionForOrigin(), which retrieves the
content setting value for a ContentSettingsType for a given origin via the
PermissionManager. It only adds Javascript test coverage for the new code, so
add test coverage for the C++ side in this patch.

BUG=656758
==========

to

==========
MD Settings: Amend SiteSettingsHandlerTest.Origins to test "Site Details" code.

https://crrev.com/2936003003 introduced
SiteSettingsHandler::HandleGetCategoryPermissionForOrigin(), which retrieves the
content setting value for a ContentSettingsType for a given origin via the
PermissionManager. It only adds Javascript test coverage for the new code, so
add test coverage for the C++ side in this patch.

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

[Patti Lor, 2017-06-29 01:49:56]

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

[commit-bot: I haz the power, 2017-06-29 01:50:27]

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, 2017-06-29 01:53:36]

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

[commit-bot: I haz the power, 2017-06-29 01:53:37]

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

[Patti Lor, 2017-06-29 01:57:37]

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

[commit-bot: I haz the power, 2017-06-29 01:57:54]

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, 2017-06-29 02:50:04]

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

[commit-bot: I haz the power, 2017-06-29 02:50:05]

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

[Patti Lor, 2017-06-29 04:01:22]

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

[commit-bot: I haz the power, 2017-06-29 04:01:36]

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, 2017-06-29 05:00:35]

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

[commit-bot: I haz the power, 2017-06-29 05:00:39]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Patti Lor, 2017-06-30 02:35:46]

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

[commit-bot: I haz the power, 2017-06-30 02:36:05]

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, 2017-06-30 03:43:07]

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

[commit-bot: I haz the power, 2017-06-30 03:43:08]

Dry run: This issue passed the CQ dry run.

[Patti Lor, 2017-06-30 06:03:55]

Description was changed from

==========
MD Settings: Amend SiteSettingsHandlerTest.Origins to test "Site Details" code.

https://crrev.com/2936003003 introduced
SiteSettingsHandler::HandleGetCategoryPermissionForOrigin(), which retrieves the
content setting value for a ContentSettingsType for a given origin via the
PermissionManager. It only adds Javascript test coverage for the new code, so
add test coverage for the C++ side in this patch.

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

to

==========
MD Settings: Amend SiteSettingsHandlerTest.Origins to test "Site Details" code.

r483614 introduced SiteSettingsHandler::HandleGetOriginPermissions(), which
retrieves the content setting value for a list of ContentSettingsTypes for a
given origin via the PermissionManager. It only adds Javascript test coverage
for the new code, so add test coverage for the C++ side in this patch.

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

[Patti Lor, 2017-07-03 06:32:07]

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

[commit-bot: I haz the power, 2017-07-03 06:32:15]

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, 2017-07-03 07:48:40]

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

[commit-bot: I haz the power, 2017-07-03 07:48:40]

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

[Patti Lor, 2017-07-03 07:52:30]

Hi dschuyler, PTAL - I've rebased back onto TOT and updated the CL description
as well.

[Patti Lor, 2017-07-05 02:53:54]

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

[commit-bot: I haz the power, 2017-07-05 02:54:04]

Dry run: CQ is trying da patch.

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

[Patti Lor, 2017-07-05 03:35:07]

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

[Patti Lor, 2017-07-05 03:35:12]

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

[commit-bot: I haz the power, 2017-07-05 03:35:20]

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, 2017-07-05 04:13:21]

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

[commit-bot: I haz the power, 2017-07-05 04:13:22]

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

[dschuyler, 2017-07-05 18:30:19]

https://codereview.chromium.org/2946393002/diff/140001/chrome/browser/resourc...
File
chrome/browser/resources/settings/site_settings/site_settings_prefs_browser_proxy.js
(right):

https://codereview.chromium.org/2946393002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/site_settings/site_settings_prefs_browser_proxy.js:161:
getOriginPermissions(origin, contentTypes) {}
It looks like this is being added a second time (maybe a merge whoops).

[Patti Lor, 2017-07-06 01:14:59]

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

[commit-bot: I haz the power, 2017-07-06 01:15:16]

Dry run: CQ is trying da patch.

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

[dschuyler, 2017-07-06 01:17:06]

Aside from a suggestion and a nit, LGTM.

https://codereview.chromium.org/2946393002/diff/140001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/settings/site_settings_handler_unittest.cc (right):

https://codereview.chromium.org/2946393002/diff/140001/chrome/browser/ui/webu...
chrome/browser/ui/webui/settings/site_settings_handler_unittest.cc:317: // Ask
is the current built-in Chrome default for notifications.
Just a thought, but I had to read this a couple times to 'get-it'. It may help a
future reader highlight "Ask" and Notifications a bit:

// "Ask" is the default value for Notifications.

[Patti Lor, 2017-07-06 01:24:27]

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

[commit-bot: I haz the power, 2017-07-06 01:24:37]

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, 2017-07-06 01:56:10]

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

[commit-bot: I haz the power, 2017-07-06 01:56:11]

Dry run: This issue passed the CQ dry run.

[Patti Lor, 2017-07-06 04:23:40]

Thanks for the review!

https://codereview.chromium.org/2946393002/diff/140001/chrome/browser/resourc...
File
chrome/browser/resources/settings/site_settings/site_settings_prefs_browser_proxy.js
(right):

https://codereview.chromium.org/2946393002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/site_settings/site_settings_prefs_browser_proxy.js:161:
getOriginPermissions(origin, contentTypes) {}
On 2017/07/05 18:30:18, dschuyler wrote:
> It looks like this is being added a second time (maybe a merge whoops).

Oops, thanks for picking this up! Fixed.

https://codereview.chromium.org/2946393002/diff/140001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/settings/site_settings_handler_unittest.cc (right):

https://codereview.chromium.org/2946393002/diff/140001/chrome/browser/ui/webu...
chrome/browser/ui/webui/settings/site_settings_handler_unittest.cc:317: // Ask
is the current built-in Chrome default for notifications.
On 2017/07/06 01:17:06, dschuyler wrote:
> Just a thought, but I had to read this a couple times to 'get-it'. It may help
a
> future reader highlight "Ask" and Notifications a bit:
> 
> // "Ask" is the default value for Notifications.

Done, thanks!

[Patti Lor, 2017-07-06 04:23:46]

The CQ bit was checked by patricialor@chromium.org

[Patti Lor, 2017-07-06 04:23:47]

The patchset sent to the CQ was uploaded after l-g-t-m from raymes@chromium.org,
dschuyler@chromium.org
Link to the patchset: https://codereview.chromium.org/2946393002/#ps180001
(title: "Fix comment.")

[commit-bot: I haz the power, 2017-07-06 04:23:59]

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, 2017-07-06 04:27:38]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1499315026962950,
"parent_rev": "a0091532daf279a05f878bf9e53bd3fad1f0dbb6", "commit_rev":
"d265af292d6342a1205dc206c7d1f9e87f0a2ce1"}

[commit-bot: I haz the power, 2017-07-06 04:27:53]

Description was changed from

==========
MD Settings: Amend SiteSettingsHandlerTest.Origins to test "Site Details" code.

r483614 introduced SiteSettingsHandler::HandleGetOriginPermissions(), which
retrieves the content setting value for a list of ContentSettingsTypes for a
given origin via the PermissionManager. It only adds Javascript test coverage
for the new code, so add test coverage for the C++ side in this patch.

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

to

==========
MD Settings: Amend SiteSettingsHandlerTest.Origins to test "Site Details" code.

r483614 introduced SiteSettingsHandler::HandleGetOriginPermissions(), which
retrieves the content setting value for a list of ContentSettingsTypes for a
given origin via the PermissionManager. It only adds Javascript test coverage
for the new code, so add test coverage for the C++ side in this patch.

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

Review-Url: https://codereview.chromium.org/2946393002
Cr-Commit-Position: refs/heads/master@{#484465}
Committed:
https://chromium.googlesource.com/chromium/src/+/d265af292d6342a1205dc206c7d1...
==========

[commit-bot: I haz the power, 2017-07-06 04:27:55]

Committed patchset #10 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/d265af292d6342a1205dc206c7d1...