[ios clean] ToolsMenu UI changes

[ios clean] ToolsMenu UI changes

Changes the Menu UI to look approximately like the current iOS version.

Screenshots:
iOS Clean: https://drive.google.com/open?id=0Byo6-Nuda2jgcUsxU09OQnRWNTQ
Current App: https://drive.google.com/open?id=0Byo6-Nuda2jgQ3JOazRNaEh1Qms

BUG=682880
Review-Url: https://codereview.chromium.org/2685823002
Cr-Commit-Position: refs/heads/master@{#449997}
Committed: https://chromium.googlesource.com/chromium/src/+/8733b51b07e8bd99fb80058e3bffc0c39ece1f1d

[sczs, 2017-02-10 23:45:40]

Description was changed from

==========
WIP Tools Menu UI

BUG=
==========

to

==========
[ios clean] ToolsMenu UI changes

BUG=682880
==========

[sczs, 2017-02-10 23:45:40]

sczs@chromium.org changed reviewers:
+ edchin@chromium.org, lpromero@chromium.org, marq@chromium.org

[sczs, 2017-02-10 23:45:47]

Patchset #1 (id:1) has been deleted

[sczs, 2017-02-10 23:52:17]

Description was changed from

==========
[ios clean] ToolsMenu UI changes

BUG=682880
==========

to

==========
[ios clean] ToolsMenu UI changes

Changes the CL so the Menu Fixed items look approximately like the current iOS
version.

Screenshots:
iOS Clean: https://drive.google.com/open?id=0Byo6-Nuda2jgcUsxU09OQnRWNTQ
Current App: https://drive.google.com/open?id=0Byo6-Nuda2jgQ3JOazRNaEh1Qms

BUG=682880
==========

[sczs, 2017-02-10 23:56:53]

Description was changed from

==========
[ios clean] ToolsMenu UI changes

Changes the CL so the Menu Fixed items look approximately like the current iOS
version.

Screenshots:
iOS Clean: https://drive.google.com/open?id=0Byo6-Nuda2jgcUsxU09OQnRWNTQ
Current App: https://drive.google.com/open?id=0Byo6-Nuda2jgQ3JOazRNaEh1Qms

BUG=682880
==========

to

==========
[ios clean] ToolsMenu UI changes

Changes the Menu UI to look approximately like the current iOS version.

Screenshots:
iOS Clean: https://drive.google.com/open?id=0Byo6-Nuda2jgcUsxU09OQnRWNTQ
Current App: https://drive.google.com/open?id=0Byo6-Nuda2jgQ3JOazRNaEh1Qms

BUG=682880
==========

[sczs, 2017-02-10 23:57:40]

PTAL

[edchin, 2017-02-11 00:00:25]

LGTM. a couple nits below.

https://codereview.chromium.org/2685823002/diff/20001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/presenters/menu_presentation_controller.mm
(right):

https://codereview.chromium.org/2685823002/diff/20001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/presenters/menu_presentation_controller.mm:54:
[self.presentedView.layer setShadowOpacity:0.4f];
nit: Make this line same as others. (dot vs. bracket notation)

https://codereview.chromium.org/2685823002/diff/20001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/presenters/menu_presentation_controller.mm:55:
self.presentedView.layer.shadowOffset = CGSizeMake(0, 0);
nit: Other floats here use the form 0.0f.

[sczs, 2017-02-11 00:04:15]

Thanks, took care of the nits!

https://codereview.chromium.org/2685823002/diff/20001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/presenters/menu_presentation_controller.mm
(right):

https://codereview.chromium.org/2685823002/diff/20001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/presenters/menu_presentation_controller.mm:54:
[self.presentedView.layer setShadowOpacity:0.4f];
On 2017/02/11 00:00:25, edchin wrote:
> nit: Make this line same as others. (dot vs. bracket notation)

Done.

https://codereview.chromium.org/2685823002/diff/20001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/presenters/menu_presentation_controller.mm:55:
self.presentedView.layer.shadowOffset = CGSizeMake(0, 0);
On 2017/02/11 00:00:25, edchin wrote:
> nit: Other floats here use the form 0.0f. 

Done.

[marq (ping after 24h), 2017-02-13 12:51:09]

LGTM after fixing RTL.

https://codereview.chromium.org/2685823002/diff/40001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/tools/menu_view_controller.mm (right):

https://codereview.chromium.org/2685823002/diff/40001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/tools/menu_view_controller.mm:102:
setTextAlignment:(!base::i18n::IsRTL() ? NSTextAlignmentRight
TL;DR: Just use NSTextAlignmentNatural.

Here are some hints for getting RTL stuff right:

(1) If you are writing conditional logic during view layout, you are probably
doing it wrong. Look at the functions and data structure in
ios/chrome/browser/ui/rtl_geometry.h (and how they are used elsewhere in the
code) for alternatives.

(2) If you absolutely have to check and see if the current layout needs to be
RTL, use UseRTLLayout() from that file. It currently just calls IsRTL(), but it
also provides an injection point for us to handle other cases -- for example,
this is where we handled the RTL pseudolanguage setting before IsRTL() did. All
of the /ios/ RTL APIs use UseRTLLayout(), so until we change them, our code
should, too.

(3) If you are dealing with UIKit layout constants, many of them have RTL-aware
variants. For NSTextAlignment, the value NSTextAlignmentNatural will align
correctly based on the current language's text direction.

[sczs, 2017-02-13 16:40:42]

https://codereview.chromium.org/2685823002/diff/40001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/tools/menu_view_controller.mm (right):

https://codereview.chromium.org/2685823002/diff/40001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/tools/menu_view_controller.mm:102:
setTextAlignment:(!base::i18n::IsRTL() ? NSTextAlignmentRight
On 2017/02/13 12:51:09, marq wrote:
> TL;DR: Just use NSTextAlignmentNatural.
> 
> Here are some hints for getting RTL stuff right:
> 
> (1) If you are writing conditional logic during view layout, you are probably
> doing it wrong. Look at the functions and data structure in
> ios/chrome/browser/ui/rtl_geometry.h (and how they are used elsewhere in the
> code) for alternatives.
> 
> (2) If you absolutely have to check and see if the current layout needs to be
> RTL, use UseRTLLayout() from that file. It currently just calls IsRTL(), but
it
> also provides an injection point for us to handle other cases -- for example,
> this is where we handled the RTL pseudolanguage setting before IsRTL() did.
All
> of the /ios/ RTL APIs use UseRTLLayout(), so until we change them, our code
> should, too.
> 
> (3) If you are dealing with UIKit layout constants, many of them have
RTL-aware
> variants. For NSTextAlignment, the value NSTextAlignmentNatural will align
> correctly based on the current language's text direction.

Thanks for the tips Mark. I actually copied this from the old ToolMenuVC, so
I'll be on the lookout for RTL conditional logic during view layout.

[sczs, 2017-02-13 16:41:05]

The CQ bit was checked by sczs@chromium.org

[sczs, 2017-02-13 16:41:05]

The patchset sent to the CQ was uploaded after l-g-t-m from edchin@chromium.org,
marq@chromium.org
Link to the patchset: https://codereview.chromium.org/2685823002/#ps60001
(title: "Use NSTextAlignmentNatural for RTL")

[commit-bot: I haz the power, 2017-02-13 16:41:36]

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-02-13 17:49:50]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1487004065297080,
"parent_rev": "745f6eed7332f26d920d0dc71b93cda281860543", "commit_rev":
"8733b51b07e8bd99fb80058e3bffc0c39ece1f1d"}

[commit-bot: I haz the power, 2017-02-13 17:50:25]

Description was changed from

==========
[ios clean] ToolsMenu UI changes

Changes the Menu UI to look approximately like the current iOS version.

Screenshots:
iOS Clean: https://drive.google.com/open?id=0Byo6-Nuda2jgcUsxU09OQnRWNTQ
Current App: https://drive.google.com/open?id=0Byo6-Nuda2jgQ3JOazRNaEh1Qms

BUG=682880
==========

to

==========
[ios clean] ToolsMenu UI changes

Changes the Menu UI to look approximately like the current iOS version.

Screenshots:
iOS Clean: https://drive.google.com/open?id=0Byo6-Nuda2jgcUsxU09OQnRWNTQ
Current App: https://drive.google.com/open?id=0Byo6-Nuda2jgQ3JOazRNaEh1Qms

BUG=682880

Review-Url: https://codereview.chromium.org/2685823002
Cr-Commit-Position: refs/heads/master@{#449997}
Committed:
https://chromium.googlesource.com/chromium/src/+/8733b51b07e8bd99fb80058e3bff...
==========

[commit-bot: I haz the power, 2017-02-13 17:50:26]

Committed patchset #3 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/8733b51b07e8bd99fb80058e3bff...