WebUI: Make settings-action-menu re-usable as cr-action-menu.

WebUI: Make settings-action-menu re-usable as cr-action-menu.

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

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

[dpapad, 2016-10-28 23:20:53]

Description was changed from

==========
WebUI: Making settings-action-menu re-usable as cr-action-menu.

BUG=658821
==========

to

==========
WebUI: Making settings-action-menu re-usable as cr-action-menu.

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

[dpapad, 2016-10-28 23:59:45]

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

[commit-bot: I haz the power, 2016-10-29 00:00:11]

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

[dpapad, 2016-10-29 00:00:24]

Description was changed from

==========
WebUI: Making settings-action-menu re-usable as cr-action-menu.

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

to

==========
WebUI: Make settings-action-menu re-usable as cr-action-menu.

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

[dpapad, 2016-10-29 00:36:13]

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

[dpapad, 2016-10-29 00:36:28]

Patchset #2 (id:20001) has been deleted

[dpapad, 2016-10-29 00:36:37]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2016-10-29 00:36:42]

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

[dpapad, 2016-10-29 00:55:41]

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

[dpapad, 2016-10-29 00:55:41]

This is the 1st step. Next steps (in follow up CLs):
 - Migrate MD History from cr-shared-menu to cr-action-menu. 
 - Delete cr-shared-menu.

[commit-bot: I haz the power, 2016-10-29 01:21:17]

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

[commit-bot: I haz the power, 2016-10-29 01:21:17]

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

[dpapad, 2016-10-31 19:59:16]

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

[commit-bot: I haz the power, 2016-10-31 19:59:53]

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, 2016-10-31 21:02:48]

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

[commit-bot: I haz the power, 2016-10-31 21:02:50]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[dpapad, 2016-10-31 22:03:02]

Removing the direction_delegate dependency from teh action menu, has resulted in
some deps failures in test (i18n_setup), trying to figure this one out..

[dpapad, 2016-10-31 23:13:25]

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

[commit-bot: I haz the power, 2016-10-31 23:14:00]

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

[dpapad, 2016-10-31 23:16:51]

On 2016/10/31 at 22:03:02, dpapad wrote:
> Removing the direction_delegate dependency from teh action menu, has resulted
in some deps failures in test (i18n_setup), trying to figure this one out..

The missing dep has been found. Expecting green tests at patch 3.

[commit-bot: I haz the power, 2016-11-01 00:19:45]

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

[commit-bot: I haz the power, 2016-11-01 00:19:46]

Dry run: This issue passed the CQ dry run.

[Dan Beam, 2016-11-01 18:13:00]

https://codereview.chromium.org/2461113002/diff/80001/chrome/test/data/webui/...
File chrome/test/data/webui/cr_elements/cr_action_menu_test.js (right):

https://codereview.chromium.org/2461113002/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/cr_elements/cr_action_menu_test.js:102: });
how do you plan to test RTL now that you're using isRTL()?

[dpapad, 2016-11-01 18:31:03]

https://codereview.chromium.org/2461113002/diff/80001/chrome/test/data/webui/...
File chrome/test/data/webui/cr_elements/cr_action_menu_test.js (right):

https://codereview.chromium.org/2461113002/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/cr_elements/cr_action_menu_test.js:102: });
On 2016/11/01 at 18:13:00, Dan Beam wrote:
> how do you plan to test RTL now that you're using isRTL()?

My thinking was the following:
Currently we don't test for RTL (even before this CL), so the direction_delegate
dependency was not benefiting us in any way yet, it was simply making the
settings -> cr migration harder. So, I am using isRTL() to unblock the
migration, and have one canonical action menu across WebUI. Then we can either
figure out a way to migrate direction_delagate and add RTL tests or just mock
util.js#isRTL() during testing. FWIW, I could not find an cr_elemnts that test
for RTL already.

[Dan Beam, 2016-11-02 16:36:12]

lgtm

https://codereview.chromium.org/2461113002/diff/80001/ui/webui/resources/cr_e...
File ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js (right):

https://codereview.chromium.org/2461113002/diff/80001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js:125: if
(isRTL()) {
can we just call getComputedStyle(anchorElement).direction == 'rtl' instead? 
that'd make this far easier to test

[Dan Beam, 2016-11-02 16:37:27]

also something else to think about (that I haven't talked a lot about yet):
<dialog> is only implemented in Chrome, but it's also part of HTML.  things that
stay implemented only in 1 browser often don't stay in the spec.  if this
element is removed from the spec, it's possible it gets removed from Chrome.  we
should probably talk to the web platform team about this.  we're starting to use
<dialog> for like everything.

[dpapad, 2016-11-02 17:24:13]

+1 on talking to the platform team about dialog. We've identified a few cases
for dialog.

1) Regular dialog shown to the user.
2) Action menu
3) Maybe <select> replacement that is fully MD styled.
4) Sliders
+ anything that is shown in a "modal" way on top of other UI. I think those
justify the existence of <dialog> as a native element.

https://codereview.chromium.org/2461113002/diff/80001/ui/webui/resources/cr_e...
File ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js (right):

https://codereview.chromium.org/2461113002/diff/80001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js:125: if
(isRTL()) {
On 2016/11/02 at 16:36:12, Dan Beam wrote:
> can we just call getComputedStyle(anchorElement).direction == 'rtl' instead? 
that'd make this far easier to test

I am reluctant calling getComputedStyle() every time a menu needs to be shown.
Seems so much more work than necessary. getComputedStyle() forces a re-layout
AFAIU (also see [1]) as well as returns a fairly large object. Are you sure this
is worth it?

https://gist.github.com/paulirish/5d52fb081b3570c81e3a#getcomputedstyle

[Dan Beam, 2016-11-02 18:54:18]

https://codereview.chromium.org/2461113002/diff/80001/ui/webui/resources/cr_e...
File ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js (right):

https://codereview.chromium.org/2461113002/diff/80001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js:125: if
(isRTL()) {
On 2016/11/02 17:24:13, dpapad wrote:
> On 2016/11/02 at 16:36:12, Dan Beam wrote:
> > can we just call getComputedStyle(anchorElement).direction == 'rtl' instead?

> that'd make this far easier to test
> 
> I am reluctant calling getComputedStyle() every time a menu needs to be shown.
> Seems so much more work than necessary. getComputedStyle() forces a re-layout
> AFAIU (also see [1]) as well as returns a fairly large object. Are you sure
this
> is worth it?
> 
> https://gist.github.com/paulirish/5d52fb081b3570c81e3a#getcomputedstyle

i think in generic components (that don't necessarilly have the guarantee that
<html> has a dir='rtl') this makes sense.  also, Polymer doles these out like
candy on halloween

[dpapad, 2016-11-02 19:25:10]

> i think in generic components (that don't necessarilly have the guarantee that
<html> has a dir='rtl') this makes sense.  also, Polymer doles these out like
candy on halloween

Done.

[dpapad, 2016-11-02 19:27:00]

The CQ bit was checked by dpapad@chromium.org

[dpapad, 2016-11-02 19:27:01]

The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org
Link to the patchset: https://codereview.chromium.org/2461113002/#ps100001
(title: "getComputedStyle instead of util.js")

[commit-bot: I haz the power, 2016-11-02 19:28:10]

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-11-02 20:39:09]

Committed patchset #4 (id:100001)

[commit-bot: I haz the power, 2016-11-02 20:41:24]

Description was changed from

==========
WebUI: Make settings-action-menu re-usable as cr-action-menu.

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

to

==========
WebUI: Make settings-action-menu re-usable as cr-action-menu.

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

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

[commit-bot: I haz the power, 2016-11-02 20:41:26]

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