Views Page Info: Implement expanding all permissions

Views Page Info: Implement expanding all permissions

BUG=657267

[lgarron, 2017-02-24 02:41:14]

lgarron@chromium.org changed reviewers:
+ msw@chromium.org

[lgarron, 2017-02-24 02:41:14]

msw@, could you review?

[1] has a Mac screenvid that shows shows most of the behaviour.

The only difference with Views is that the initial implementation allows us to
prevent permissions from disappearing; see [2] for a MacViews screenvid that
shows this working.

[1] https://bugs.chromium.org/p/chromium/issues/detail?id=657267#c9
[2] https://bugs.chromium.org/p/chromium/issues/detail?id=695723&desc=2#c4

[msw, 2017-02-24 18:42:49]

I don't like that permissions disappear instantly when you set them to default.
It seems like they should be visible right after you interact with them, and
just not be shown the next time the bubble appears. That would match the
expected.mov from http://crbug.com/695723#c4 and probably simplify the code too.
If you pursue the content changes while the bubble is open, make sure it's okay
with UX (I'd expect that they'd also dislike things disappearing that the user
just clicked, or they'd want some kind of animation)

https://codereview.chromium.org/2717613003/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc
(right):

https://codereview.chromium.org/2717613003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:17:
#include "base/strings/sys_string_conversions.h"
Is this used?

https://codereview.chromium.org/2717613003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:108:
const int LINK_SHOW_ALL_PERMISSIONS = 1342;
optional nit: constexpr here and for others above

https://codereview.chromium.org/2717613003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:622: //
Clear all children, since it's not straightforward to intersperse new ones
nit: simplify (or remove) this comment: "Recreate the permissions view from the
new info."

https://codereview.chromium.org/2717613003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:624:
permissions_view_->RemoveAllChildViews(true);
Does this cause any flickering when permissions change while the bubble is open?

https://codereview.chromium.org/2717613003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:666: if
(visible_permissions == WebsiteSettingsUI::VISIBLE_PERMISSIONS_ALL) {
nit: curlies not needed (or use a ternary)

https://codereview.chromium.org/2717613003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:712:
permission_link_ = new views::Link(base::string16());
It would be nice if this class initialized the link/button to anything (I'd
suggest IDS_PAGE_INFO_SITE_SETTINGS_LINK / LINK_SITE_SETTINGS), rather than
leaving a blank link until something calls UpdatePermissionButton.

https://codereview.chromium.org/2717613003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:732:
presenter_->OnPresentAllSitePermissions();
nit: maybe just "PresentAllSitePermissions", it's an imperative, not a response
to something occurring (otherwise it would be
"OnShowAllPermissionsButtonPressed").

https://codereview.chromium.org/2717613003/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/website_settings/website_settings_popup_view.h
(right):

https://codereview.chromium.org/2717613003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/website_settings/website_settings_popup_view.h:142: //
Creates the contents of the |site_settings_view_|. The ownership of the
This comment is copy-pasted from above, please update it.

[lgarron, 2017-02-28 03:21:01]

Ah, I should have tried to be even more clear. `expected.mov` *is* the Views
behaviour in this implementation.

On Views, the implementation of the permission rows updates itself without a
need to call WebsiteSettings::PresentSitePermissions().

The parent CL changes WebsiteSettings() to avoid calling
WebsiteSettings::PresentSitePermissions() in Views when an individual permission
is changed in WebsiteSettings::OnSitePermissionChanged(() – see `#if
defined(OS_MACOSX)` in the parent CL at
https://codereview.chromium.org/2702923002/patch/150001/160006

See https://bugs.chromium.org/p/chromium/issues/detail?id=657267#c21 for a
screenvid.

https://codereview.chromium.org/2717613003/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc
(right):

https://codereview.chromium.org/2717613003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:17:
#include "base/strings/sys_string_conversions.h"
On 2017/02/24 at 18:42:49, msw wrote:
> Is this used?

No, certainly not in the current patch anymore. Removed.

https://codereview.chromium.org/2717613003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:108:
const int LINK_SHOW_ALL_PERMISSIONS = 1342;
On 2017/02/24 at 18:42:49, msw wrote:
> optional nit: constexpr here and for others above

I would prefer to avoid growing it in this CL, but I've filed a bug for when I
do Page Info cleanup:
https://bugs.chromium.org/p/chromium/issues/detail?id=696859

https://codereview.chromium.org/2717613003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:622: //
Clear all children, since it's not straightforward to intersperse new ones
On 2017/02/24 at 18:42:49, msw wrote:
> nit: simplify (or remove) this comment: "Recreate the permissions view from
the new info."

Sure. I've taken your comment.

https://codereview.chromium.org/2717613003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:624:
permissions_view_->RemoveAllChildViews(true);
On 2017/02/24 at 18:42:49, msw wrote:
> Does this cause any flickering when permissions change while the bubble is
open?

No. At least not on Mac, and also not the one time I tried on Linux – the new
values appear instantly.

(Also, as mentioned above, this code is not triggered for Views when changing a
permission – just when updating which permissions are shown.)

https://codereview.chromium.org/2717613003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:712:
permission_link_ = new views::Link(base::string16());
On 2017/02/24 at 18:42:49, msw wrote:
> It would be nice if this class initialized the link/button to anything (I'd
suggest IDS_PAGE_INFO_SITE_SETTINGS_LINK / LINK_SITE_SETTINGS), rather than
leaving a blank link until something calls UpdatePermissionButton.

Sure.

I've set and initial ID of LINK_SITE_SETTINGS and changed the default string to
GetPermissionButtonString(WebsiteSettingsUI::VISIBLE_PERMISSIONS_ALL), to keep
the string calculation abstraction entirely inside the cross-platform
implementation of GetPermissionButtonString. Let me know if using
IDS_PAGE_INFO_SITE_SETTINGS_LINK directly would be better practice.

https://codereview.chromium.org/2717613003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:732:
presenter_->OnPresentAllSitePermissions();
On 2017/02/24 at 18:42:48, msw wrote:
> nit: maybe just "PresentAllSitePermissions", it's an imperative, not a
response to something occurring (otherwise it would be
"OnShowAllPermissionsButtonPressed").

Oh, I see. Yeah, I definitely prefer PresentAllSitePermissions(); I didn't
realize the others actually all matched an action.

https://codereview.chromium.org/2717613003/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/website_settings/website_settings_popup_view.h
(right):

https://codereview.chromium.org/2717613003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/website_settings/website_settings_popup_view.h:142: //
Creates the contents of the |site_settings_view_|. The ownership of the
On 2017/02/24 at 18:42:49, msw wrote:
> This comment is copy-pasted from above, please update it.

Done.

[msw, 2017-02-28 18:35:18]

I think my only remaining concern is the visual jank caused when changing the
widest permissions rows, expressed in http://crbug.com/657267#c24