Fix content_setting_image_view compile on mac_views_browser

Fix content_setting_image_view compile on mac_views_browser

This broke in r361436 when adding vector icon support because methods
that the views code requires from the model were hidden on Mac. When
mac_views_browser=1 is set in GYP_DEFINES, the toolkit-views browser's
content_setting_image_view.cc is part of the build, and it wasn't able
to see methods such as ContentSettingImageModel::GetIcon() on Mac.

To use content_setting_image_view on Mac in the transition, it needs to
support both Views and Cocoa on Mac (at least for compiling).

BUG=425229
Committed: https://crrev.com/cbc7360e60b8f0983867c44a0813c6a08d693afc
Cr-Commit-Position: refs/heads/master@{#363820}

[tapted, 2015-12-07 07:57:18]

Description was changed from

==========
Fix content_setting_image_view compile on Mac

This broke in r361436 when adding vector icon support because methods
that the views code requires from the model were hidden on Mac.

To use content_setting_image_view on Mac in the transition, it needs to
support both Views and Cocoa on Mac (at least for compiling).

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

to

==========
Fix content_setting_image_view compile on Mac

This broke in r361436 when adding vector icon support because methods
that the views code requires from the model were hidden on Mac.

To use content_setting_image_view on Mac in the transition, it needs to
support both Views and Cocoa on Mac (at least for compiling).

BUG=425229
==========

[tapted, 2015-12-07 07:57:18]

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

[tapted, 2015-12-07 12:39:32]

Description was changed from

==========
Fix content_setting_image_view compile on Mac

This broke in r361436 when adding vector icon support because methods
that the views code requires from the model were hidden on Mac.

To use content_setting_image_view on Mac in the transition, it needs to
support both Views and Cocoa on Mac (at least for compiling).

BUG=425229
==========

to

==========
Fix content_setting_image_view compile on mac_views_browser

This broke in r361436 when adding vector icon support because methods
that the views code requires from the model were hidden on Mac.

To use content_setting_image_view on Mac in the transition, it needs to
support both Views and Cocoa on Mac (at least for compiling).

BUG=425229
==========

[tapted, 2015-12-07 12:45:53]

Description was changed from

==========
Fix content_setting_image_view compile on mac_views_browser

This broke in r361436 when adding vector icon support because methods
that the views code requires from the model were hidden on Mac.

To use content_setting_image_view on Mac in the transition, it needs to
support both Views and Cocoa on Mac (at least for compiling).

BUG=425229
==========

to

==========
Fix content_setting_image_view compile on mac_views_browser

This broke in r361436 when adding vector icon support because methods
that the views code requires from the model were hidden on Mac. When
mac_views_browser=1 is set in GYP_DEFINES, the toolkit-views browser's
content_setting_image_view.cc is part of the build, and it wasn't able
to see methods such as ContentSettingImageModel::GetIcon() on Mac.

To use content_setting_image_view on Mac in the transition, it needs to
support both Views and Cocoa on Mac (at least for compiling).

BUG=425229
==========

[tapted, 2015-12-07 12:46:25]

Hi Evan, please take a look

[Evan Stade, 2015-12-07 20:01:28]

I feel like the right define here would be TOOLKIT_VIEWS but for some reason
TOOLKIT_VIEW is true even on non-views mac. Can we fix that?

https://codereview.chromium.org/1482183002/diff/80001/chrome/browser/ui/conte...
File chrome/browser/ui/content_settings/content_setting_image_model.h (left):

https://codereview.chromium.org/1482183002/diff/80001/chrome/browser/ui/conte...
chrome/browser/ui/content_settings/content_setting_image_model.h:52: #if
defined(OS_MACOSX)
can we leave this one? I don't want people using it by mistake (you can get rid
of the #else)

[tapted, 2015-12-08 01:50:10]

On 2015/12/07 20:01:28, Evan Stade wrote:
> I feel like the right define here would be TOOLKIT_VIEWS but for some reason
> TOOLKIT_VIEW is true even on non-views mac. Can we fix that?

Yup - TOOLKIT_VIEWS is true, because there's already TOOLKIT_VIEWS code on Mac
behind runtime flags (e.g. chrome://flags/#mac-views-dialogs
chrome://flags/#app-info-dialog ). So a compile-time option won't work for them.

Of course, this CL is about "mac_views_browser=1" being set in GYP_DEFINES (not
toolkit_views). I'm very reluctant to feed in a matching #define for
mac_views_browser. The problem is that it can only be temporary, and it's not
really needed. That is, apart from these small regressions a "Cocoa-less" 
toolkit-views browser on Mac already works (it compiles and is quite usable).
Adding a #define to fix a regression doesn't feel right, and exactly what
"MAC_VIEWS_BROWSER" means is confusing.

mac_views_browser=1 basically says "compile toolkit-views' browser_view.cc" (and
all the stuff that goes with it). It's currently done entirely in gyp. But while
browser_view.cc isn't included with mac_views_browser=0, other dialogs under
c/b/ui *are* included by default (i.e. those currently behind runtime flags).
So, if someone tried to put MAC_VIEWS_BROWSER in a header file, it kinda needs
to change depending which dialog is being compiled. i.e. it would be really hard
to use the #define "right", and to reason about code using it in isolation.

https://codereview.chromium.org/1482183002/diff/80001/chrome/browser/ui/conte...
File chrome/browser/ui/content_settings/content_setting_image_model.h (left):

https://codereview.chromium.org/1482183002/diff/80001/chrome/browser/ui/conte...
chrome/browser/ui/content_settings/content_setting_image_model.h:52: #if
defined(OS_MACOSX)
On 2015/12/07 20:01:28, Evan Stade wrote:
> can we leave this one? I don't want people using it by mistake (you can get
rid
> of the #else)

Ah good idea :). Done.

[Evan Stade, 2015-12-08 01:54:10]

https://codereview.chromium.org/1482183002/diff/100001/chrome/browser/ui/cont...
File chrome/browser/ui/content_settings/content_setting_image_model.cc (right):

https://codereview.chromium.org/1482183002/diff/100001/chrome/browser/ui/cont...
chrome/browser/ui/content_settings/content_setting_image_model.cc:29: return
false;
don't you want this to be true for mac views browser? It may compile like this
but then content_setting_image_view.cc, which relies on vector graphics
exclusively, won't work properly.

[tapted, 2015-12-08 02:18:22]

https://codereview.chromium.org/1482183002/diff/100001/chrome/browser/ui/cont...
File chrome/browser/ui/content_settings/content_setting_image_model.cc (right):

https://codereview.chromium.org/1482183002/diff/100001/chrome/browser/ui/cont...
chrome/browser/ui/content_settings/content_setting_image_model.cc:29: return
false;
On 2015/12/08 01:54:10, Evan Stade wrote:
> don't you want this to be true for mac views browser? It may compile like this
> but then content_setting_image_view.cc, which relies on vector graphics
> exclusively, won't work properly.

I just tried it out at r363391 (plus this CL) - it works OK at this point
(http://i.imgur.com/aekXs4K.jpg) -- I think the non-material mode still needs to
be supported, even on views for now?

When content_setting_image_view.cc needs to rely on vector graphics exclusively,
it's OK to break mac_views_browser -- it's not released, so it doesn't matter if
parts break. We just need something that compiles and runs so we can use it to
make decisions on the general design and make progress on how the browser might
behave in a Cocoa-free world.

I think when content_setting_image_view.cc needs to rely on vector graphics
exclusively, the dialog code will need a way to ask for vector graphics
regardless. When we want to test content_setting_image_view.cc on Mac, we'll
need to put it behind a runtime flag, which means both Cocoa and Views code
could potentially ask for it in a build. I.e. the "UseVectorGraphics()" would
effectively move either to the toolkit-specific code, or it can check here based
on a runtime flag, but it wouldn't be able to come from a #define.

[Evan Stade, 2015-12-08 02:21:25]

On 2015/12/08 02:18:22, tapted wrote:
>
https://codereview.chromium.org/1482183002/diff/100001/chrome/browser/ui/cont...
> File chrome/browser/ui/content_settings/content_setting_image_model.cc
(right):
> 
>
https://codereview.chromium.org/1482183002/diff/100001/chrome/browser/ui/cont...
> chrome/browser/ui/content_settings/content_setting_image_model.cc:29: return
> false;
> On 2015/12/08 01:54:10, Evan Stade wrote:
> > don't you want this to be true for mac views browser? It may compile like
this
> > but then content_setting_image_view.cc, which relies on vector graphics
> > exclusively, won't work properly.
> 
> I just tried it out at r363391 (plus this CL) - it works OK at this point
> (http://i.imgur.com/aekXs4K.jpg) -- I think the non-material mode still needs
to
> be supported, even on views for now?

I think it would be broken if you pass --top-chrome-md=material. Anyway, if you
don't mind that, lgtm

[tapted, 2015-12-08 02:28:53]

On 2015/12/08 02:21:25, Evan Stade wrote:
> I think it would be broken if you pass --top-chrome-md=material. Anyway, if
you
> don't mind that, lgtm

yah - still taking baby steps for views on mac :)

[tapted, 2015-12-08 02:29:01]

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

[tapted, 2015-12-08 02:29:02]

+bauerb - PTAL for c/b/ui/content_settings OWNERS

[Bernhard Bauer, 2015-12-08 10:12:52]

lgtm

[tapted, 2015-12-08 11:50:05]

The CQ bit was checked by tapted@chromium.org

[commit-bot: I haz the power, 2015-12-08 11:50:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1482183002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1482183002/100001

[commit-bot: I haz the power, 2015-12-08 23:21:23]

Description was changed from

==========
Fix content_setting_image_view compile on mac_views_browser

This broke in r361436 when adding vector icon support because methods
that the views code requires from the model were hidden on Mac. When
mac_views_browser=1 is set in GYP_DEFINES, the toolkit-views browser's
content_setting_image_view.cc is part of the build, and it wasn't able
to see methods such as ContentSettingImageModel::GetIcon() on Mac.

To use content_setting_image_view on Mac in the transition, it needs to
support both Views and Cocoa on Mac (at least for compiling).

BUG=425229
==========

to

==========
Fix content_setting_image_view compile on mac_views_browser

This broke in r361436 when adding vector icon support because methods
that the views code requires from the model were hidden on Mac. When
mac_views_browser=1 is set in GYP_DEFINES, the toolkit-views browser's
content_setting_image_view.cc is part of the build, and it wasn't able
to see methods such as ContentSettingImageModel::GetIcon() on Mac.

To use content_setting_image_view on Mac in the transition, it needs to
support both Views and Cocoa on Mac (at least for compiling).

BUG=425229
==========

[commit-bot: I haz the power, 2015-12-08 23:21:24]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-12-08 23:22:49]

Description was changed from

==========
Fix content_setting_image_view compile on mac_views_browser

This broke in r361436 when adding vector icon support because methods
that the views code requires from the model were hidden on Mac. When
mac_views_browser=1 is set in GYP_DEFINES, the toolkit-views browser's
content_setting_image_view.cc is part of the build, and it wasn't able
to see methods such as ContentSettingImageModel::GetIcon() on Mac.

To use content_setting_image_view on Mac in the transition, it needs to
support both Views and Cocoa on Mac (at least for compiling).

BUG=425229
==========

to

==========
Fix content_setting_image_view compile on mac_views_browser

This broke in r361436 when adding vector icon support because methods
that the views code requires from the model were hidden on Mac. When
mac_views_browser=1 is set in GYP_DEFINES, the toolkit-views browser's
content_setting_image_view.cc is part of the build, and it wasn't able
to see methods such as ContentSettingImageModel::GetIcon() on Mac.

To use content_setting_image_view on Mac in the transition, it needs to
support both Views and Cocoa on Mac (at least for compiling).

BUG=425229

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

[commit-bot: I haz the power, 2015-12-08 23:22:50]

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