MacViews: fix bubble direction in RTL.

MacViews: fix bubble direction in RTL.

The Cocoa browser doesn't have good RTL support, and doesn't flip its UI
in RTL. Bubble direction should not flip by default.

Add BUILDFLAG(MAC_VIEWS_BROWSER) to accomplish this.

BUG=657244
Committed: https://crrev.com/c60452305f151964dee9b5eaff7c9b0356c41c16
Cr-Commit-Position: refs/heads/master@{#435207}

[tapted, 2016-11-25 03:10:43]

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

[commit-bot: I haz the power, 2016-11-25 03:10: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, 2016-11-25 03:21:54]

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

[commit-bot: I haz the power, 2016-11-25 03:21:55]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)

[tapted, 2016-11-28 07:49:52]

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

[commit-bot: I haz the power, 2016-11-28 07:50:05]

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

[tapted, 2016-11-28 11:35:06]

tapted@chromium.org changed reviewers:
+ sky@chromium.org

[tapted, 2016-11-28 11:35:06]

Hi sky - please take a look.

[commit-bot: I haz the power, 2016-11-28 11:39:11]

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

[commit-bot: I haz the power, 2016-11-28 11:39:12]

Dry run: This issue passed the CQ dry run.

[sky, 2016-11-28 16:22:22]

https://codereview.chromium.org/2532613002/diff/40001/ui/views/BUILD.gn
File ui/views/BUILD.gn (right):

https://codereview.chromium.org/2532613002/diff/40001/ui/views/BUILD.gn#newco...
ui/views/BUILD.gn:624: if (mac_views_browser) {
Why does whether the browser uses views influence the direction?

Assuming that is valid I would prefer not to have files with the names you list
here. Can platform_style.cc and platform_style_mac handled the conditionals?
Does mac_views_browser map to a buildflag? If not, can it?

[tapted, 2016-11-28 23:51:06]

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

[commit-bot: I haz the power, 2016-11-28 23:52:03]

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

[tapted, 2016-11-29 00:30:15]

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

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

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-11-29 02:49:45]

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

[commit-bot: I haz the power, 2016-11-29 02:49:45]

Dry run: This issue passed the CQ dry run.

[tapted, 2016-11-29 06:43:22]

Description was changed from

==========
MacViews: fix bubble direciton in RTL.

The Cocoa browser doesn't have good RTL support, and doesn't flip its UI
in RTL. Bubble direction should not flip by default.

BUG=657244
==========

to

==========
MacViews: fix bubble direciton in RTL.

The Cocoa browser doesn't have good RTL support, and doesn't flip its UI
in RTL. Bubble direction should not flip by default.

Add BUILDFLAG(MAC_VIEWS_BROWSER) to accomplish this.

BUG=657244
==========

[tapted, 2016-11-29 06:43:40]

Description was changed from

==========
MacViews: fix bubble direciton in RTL.

The Cocoa browser doesn't have good RTL support, and doesn't flip its UI
in RTL. Bubble direction should not flip by default.

Add BUILDFLAG(MAC_VIEWS_BROWSER) to accomplish this.

BUG=657244
==========

to

==========
MacViews: fix bubble direction in RTL.

The Cocoa browser doesn't have good RTL support, and doesn't flip its UI
in RTL. Bubble direction should not flip by default.

Add BUILDFLAG(MAC_VIEWS_BROWSER) to accomplish this.

BUG=657244
==========

[tapted, 2016-11-29 06:43:54]

https://codereview.chromium.org/2532613002/diff/40001/ui/views/BUILD.gn
File ui/views/BUILD.gn (right):

https://codereview.chromium.org/2532613002/diff/40001/ui/views/BUILD.gn#newco...
ui/views/BUILD.gn:624: if (mac_views_browser) {
On 2016/11/28 16:22:22, sky wrote:
> Why does whether the browser uses views influence the direction?

We never implemented proper RTL support for the Cocoa browser window :/. There
is `--enable-features=ExperimentalMacRTL` but it currently just flips the tab
strip and window frame. Flipping the toolbar is a tougher prospect (hopefully we
will switch straight to a toolkit-views browser for that).

So, e.g. for RTL on Mac, the bookmark star, extensions, etc. all remain on the
right, and the padlock remains on the left. This accounts for all the bubbles we
care about right now. (an exception would be the profile switcher - it should
open to the right in RTL, but we haven't ported that to toolkit-views).

This means that if we _do_ flip the bubble direction, the bubble opens towards
the outside of the browser window, rather than towards the inside of the browser
window.

> 
> Assuming that is valid I would prefer not to have files with the names you
list
> here. Can platform_style.cc and platform_style_mac handled the conditionals?
> Does mac_views_browser map to a buildflag? If not, can it?

It doesn't currently have a BUILDFLAG. I've added one (Done.).

There are some downsides of that (I've been resisting it for a while..), but
they're no longer as bad as the old #ifdef approach, so I guess I'm sold it it
:).

[sky, 2016-11-29 14:12:48]

LGTM

[lgrey, 2016-11-29 14:55:34]

On 2016/11/29 06:43:54, tapted wrote:
> 
> We never implemented proper RTL support for the Cocoa browser window :/. There
> is `--enable-features=ExperimentalMacRTL` but it currently just flips the tab
> strip and window frame. Flipping the toolbar is a tougher prospect (hopefully
we
> will switch straight to a toolkit-views browser for that).

Drive-by: I'll probably be working on this next week. Probably doesn't affect
this CL, but just FYI.

[tapted, 2016-11-29 23:30:10]

On 2016/11/29 14:55:34, lgrey wrote:
> On 2016/11/29 06:43:54, tapted wrote:
> > 
> > We never implemented proper RTL support for the Cocoa browser window :/.
There
> > is `--enable-features=ExperimentalMacRTL` but it currently just flips the
tab
> > strip and window frame. Flipping the toolbar is a tougher prospect
(hopefully
> we
> > will switch straight to a toolkit-views browser for that).
> 
> Drive-by: I'll probably be working on this next week. Probably doesn't affect
> this CL, but just FYI.

Thanks for the heads-up! I'll keep an eye on it - I saw you had a CL for the
bookmarks bar. The omnibox.. it looks like it will be super tricky to fix in
Cocoa - but hit me up if you need a hand with the bubbles.

[tapted, 2016-11-29 23:31:36]

tapted@chromium.org changed reviewers:
+ scottmg@chromium.org

[tapted, 2016-11-29 23:31:51]

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

[tapted, 2016-11-29 23:33:46]

+scottmg for build/config and tools/grit/OWNERS
+msw for components/constrained_window

[scottmg, 2016-11-29 23:35:44]

lgtm

[msw, 2016-11-29 23:45:09]

components/constrained_window lgtm

[tapted, 2016-11-29 23:46:06]

The CQ bit was checked by tapted@chromium.org

[commit-bot: I haz the power, 2016-11-29 23:47:03]

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-30 03:01:11]

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

[commit-bot: I haz the power, 2016-11-30 03:01:13]

Try jobs failed on following builders:
  blimp_linux_dbg on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)

[tapted, 2016-11-30 09:44:43]

The CQ bit was checked by tapted@chromium.org

[commit-bot: I haz the power, 2016-11-30 09:45:11]

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-30 10:42:11]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1480499083459110,
"parent_rev": "1adf622daac78bb19ee771d7cb96d22d17bc7809", "commit_rev":
"f6a0b23f7f1b329ab5c0d2f4a52589a1982bb32f"}

[commit-bot: I haz the power, 2016-11-30 10:42:36]

Description was changed from

==========
MacViews: fix bubble direction in RTL.

The Cocoa browser doesn't have good RTL support, and doesn't flip its UI
in RTL. Bubble direction should not flip by default.

Add BUILDFLAG(MAC_VIEWS_BROWSER) to accomplish this.

BUG=657244
==========

to

==========
MacViews: fix bubble direction in RTL.

The Cocoa browser doesn't have good RTL support, and doesn't flip its UI
in RTL. Bubble direction should not flip by default.

Add BUILDFLAG(MAC_VIEWS_BROWSER) to accomplish this.

BUG=657244
==========

[commit-bot: I haz the power, 2016-11-30 10:42:37]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-11-30 10:44:19]

Description was changed from

==========
MacViews: fix bubble direction in RTL.

The Cocoa browser doesn't have good RTL support, and doesn't flip its UI
in RTL. Bubble direction should not flip by default.

Add BUILDFLAG(MAC_VIEWS_BROWSER) to accomplish this.

BUG=657244
==========

to

==========
MacViews: fix bubble direction in RTL.

The Cocoa browser doesn't have good RTL support, and doesn't flip its UI
in RTL. Bubble direction should not flip by default.

Add BUILDFLAG(MAC_VIEWS_BROWSER) to accomplish this.

BUG=657244

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

[commit-bot: I haz the power, 2016-11-30 10:44:20]

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

[kjellander_webrtc, 2016-12-01 07:01:39]

kjellander@webrtc.org changed reviewers:
+ kjellander@webrtc.org

[kjellander_webrtc, 2016-12-01 07:01:40]

This CL introduces dependency from tools/grit to src/ui - is there another way
to do it, since this breaks WebRTC? (see comment for more info)

https://codereview.chromium.org/2532613002/diff/80001/tools/grit/grit_rule.gni
File tools/grit/grit_rule.gni (right):

https://codereview.chromium.org/2532613002/diff/80001/tools/grit/grit_rule.gn...
tools/grit/grit_rule.gni:95: import("//ui/base/ui_features.gni")
I don't think it makes sense to have tools/grit depending on //ui in Chrome. Is
there another way to get around this?

In WebRTC, we're using the Chromium Android test framework and we pull in the
dependencies of it. With this change, our build broke since Chromium's ui/
directory is obviously not something we can have.
It broke our auto-roll here: https://codereview.webrtc.org/2542033002/

[tapted, 2016-12-01 07:52:24]

On 2016/12/01 07:01:40, kjellander_webrtc wrote:
> This CL introduces dependency from tools/grit to src/ui - is there another way
> to do it, since this breaks WebRTC? (see comment for more info)
> 
> https://codereview.chromium.org/2532613002/diff/80001/tools/grit/grit_rule.gni
> File tools/grit/grit_rule.gni (right):
> 
>
https://codereview.chromium.org/2532613002/diff/80001/tools/grit/grit_rule.gn...
> tools/grit/grit_rule.gni:95: import("//ui/base/ui_features.gni")
> I don't think it makes sense to have tools/grit depending on //ui in Chrome.
Is
> there another way to get around this?
> 
> In WebRTC, we're using the Chromium Android test framework and we pull in the
> dependencies of it. With this change, our build broke since Chromium's ui/
> directory is obviously not something we can have.
> It broke our auto-roll here: https://codereview.webrtc.org/2542033002/

Published https://codereview.chromium.org/2535753007/