[MD settings] remove action menu from all-sites

[MD settings] remove action menu from all-sites

This CL fixes a crash in content settings that arises from using the
action menu within the all-sites page. The action menu should be hidden
on that page and this CL removes that UI from the all-sites page.

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

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

[dschuyler, 2016-12-21 01:18:00]

Description was changed from

==========
[MD settings] remove action menu from all-sites

This CL fixes a crash in content settings that arises from using the
action menu within the all-sites page. The action menu should be hidden
on that page and this CL removes that UI from the all-sites page.

BUG=655929
==========

to

==========
[MD settings] remove action menu from all-sites

This CL fixes a crash in content settings that arises from using the
action menu within the all-sites page. The action menu should be hidden
on that page and this CL removes that UI from the all-sites page.

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

[dschuyler, 2016-12-21 01:18:02]

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

[commit-bot: I haz the power, 2016-12-21 01:18:29]

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

[dschuyler, 2016-12-21 01:19:08]

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

[dpapad, 2016-12-21 01:30:59]

Can we add a test at
https://cs.chromium.org/chromium/src/chrome/test/data/webui/settings/site_lis...
to ensure that the menu is hidden?

https://codereview.chromium.org/2593503004/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/site_settings/site_list.js (right):

https://codereview.chromium.org/2593503004/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/site_list.js:205:
isActionMenuHidden_: function(source) {
So it seems shouldShowMenu_ was dead code previously? Looking at the history,
last usage was removed at https://codereview.chromium.org/2386993005, but the
function itself was not removed (and I did not catch this in review).

[dschuyler, 2016-12-21 02:02:59]

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

[commit-bot: I haz the power, 2016-12-21 02:03:30]

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

[dschuyler, 2016-12-21 02:06:47]

Unit test added.

https://codereview.chromium.org/2593503004/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/site_settings/site_list.js (right):

https://codereview.chromium.org/2593503004/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/site_list.js:205:
isActionMenuHidden_: function(source) {
On 2016/12/21 01:30:59, dpapad wrote:
> So it seems shouldShowMenu_ was dead code previously? 
That appears to be so.

> Looking at the history,
> last usage was removed at https://codereview.chromium.org/2386993005, but the
> function itself was not removed (and I did not catch this in review).
Not a big deal.

https://codereview.chromium.org/2593503004/diff/20001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/site_list_tests.js (right):

https://codereview.chromium.org/2593503004/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/settings/site_list_tests.js:720: return
resolver.promise.then(function() {
I'm including this promise resolve because other tests in this file do so and it
seems to help. I'm not 100% clear on why though. Are you more familiar with the
issue here?

[dpapad, 2016-12-21 02:32:13]

https://codereview.chromium.org/2593503004/diff/20001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/site_list_tests.js (right):

https://codereview.chromium.org/2593503004/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/settings/site_list_tests.js:720: return
resolver.promise.then(function() {
On 2016/12/21 at 02:06:47, dschuyler wrote:
> I'm including this promise resolve because other tests in this file do so and
it seems to help. I'm not 100% clear on why though. Are you more familiar with
the issue here?

The comment is a bit misleading (or I am misunderstanding). I thought the
Resolver is only used to artificially yield to the message loop so that the
dom-repeat renders. Errors are reported correctly anyway, no?

Another way I was able to make it work was http://imgur.com/a/rsxHj.

Summarizing our discussion, listening for the dom-change event fired from the
dom-repeat seems promising, but might be more involved than this UI deserves.

[dschuyler, 2016-12-21 02:37:46]

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

[commit-bot: I haz the power, 2016-12-21 02:38:04]

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

[dschuyler, 2016-12-21 02:39:39]

https://codereview.chromium.org/2593503004/diff/20001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/site_list_tests.js (right):

https://codereview.chromium.org/2593503004/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/settings/site_list_tests.js:720: return
resolver.promise.then(function() {
On 2016/12/21 02:32:13, dpapad wrote:
> On 2016/12/21 at 02:06:47, dschuyler wrote:
> > I'm including this promise resolve because other tests in this file do so
and
> it seems to help. I'm not 100% clear on why though. Are you more familiar with
> the issue here?
> 
> The comment is a bit misleading (or I am misunderstanding). I thought the
> Resolver is only used to artificially yield to the message loop so that the
> dom-repeat renders. Errors are reported correctly anyway, no?
> 
> Another way I was able to make it work was http://imgur.com/a/rsxHj.
> 
> Summarizing our discussion, listening for the dom-change event fired from the
> dom-repeat seems promising, but might be more involved than this UI deserves.

Thanks for looking at that with me. Knowing when Polymer is finished startup is
a long standing issue. I like your ideas on finding better ways to do it, and I
agree that it's more than needed for this particular CL.

Changed comment.

[dpapad, 2016-12-21 03:28:33]

lgtm

[commit-bot: I haz the power, 2016-12-21 03:37:43]

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

[commit-bot: I haz the power, 2016-12-21 03:37:46]

Dry run: This issue passed the CQ dry run.

[dschuyler, 2016-12-21 17:50:21]

The CQ bit was checked by dschuyler@chromium.org

[commit-bot: I haz the power, 2016-12-21 17:50:42]

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-12-21 17:57:15]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1482342621616560,
"parent_rev": "24fafdc2bde9c5c19a92b64b66e4f492ad88366e", "commit_rev":
"bfb5c34e4c2191e9db5c77e948370322dfb64b86"}

[commit-bot: I haz the power, 2016-12-21 17:57:42]

Description was changed from

==========
[MD settings] remove action menu from all-sites

This CL fixes a crash in content settings that arises from using the
action menu within the all-sites page. The action menu should be hidden
on that page and this CL removes that UI from the all-sites page.

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

to

==========
[MD settings] remove action menu from all-sites

This CL fixes a crash in content settings that arises from using the
action menu within the all-sites page. The action menu should be hidden
on that page and this CL removes that UI from the all-sites page.

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

Review-Url: https://codereview.chromium.org/2593503004
==========

[commit-bot: I haz the power, 2016-12-21 17:57:43]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-12-21 18:00:24]

Description was changed from

==========
[MD settings] remove action menu from all-sites

This CL fixes a crash in content settings that arises from using the
action menu within the all-sites page. The action menu should be hidden
on that page and this CL removes that UI from the all-sites page.

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

Review-Url: https://codereview.chromium.org/2593503004
==========

to

==========
[MD settings] remove action menu from all-sites

This CL fixes a crash in content settings that arises from using the
action menu within the all-sites page. The action menu should be hidden
on that page and this CL removes that UI from the all-sites page.

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

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

[commit-bot: I haz the power, 2016-12-21 18:00:25]

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