MacViews: gyp change for building MacViews browser.

MacViews: gyp change for building MacViews browser.

For mac_views_browser we currently compile as if for Cocoa+toolkit_views then
remove both views files that are not ready, and cocoa files that conflict with
things already ported.

More of views compiles on Mac now than doesn't (but isn't ready to go into a
release build). This change moves those ready files back to non_mac_sources
for now. There are too many doubly defined functions that we risk linking
a Views one into the shipping Cocoa binary.

Then, for mac_views_browser, we add all the "non_mac" views files and just
remove the cocoa files that would conflict.

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

[Andre, 2015-03-05 22:38:06]

Patchset #1 (id:1) has been deleted

[Andre, 2015-03-05 22:38:58]

Patchset #1 (id:20001) has been deleted

[Andre, 2015-03-06 00:24:56]

Patchset #1 (id:40001) has been deleted

[Andre, 2015-03-06 00:43:57]

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

[Andre, 2015-03-06 00:43:57]

Trent, WDYT?
Here we compile Views only when mac_views_browser is 1.
I think there are too many of these multiply defined global functions to
safely compile both Cocoa + Views and then try to prune Views at link
time. The linker does not give a warning in most of these cases, and
one of the definition gets picked silently. I worry about breaking Cocoa
if some of the Views definition gets picked.

[tapted, 2015-03-06 02:38:57]

Yep, I think it's a good idea, but it needs some rationale in the CL
description.

E.g. 

For mac_views_browser we currently compile as if for Cocoa+toolkit_views then
remove both views files that are not ready, and cocoa files that conflict with
things already ported.

More of views compiles on Mac now than doesn't (but isn't ready to go into a
release build). This change merges chrome_browser_ui_views_non_mac_source into
chrome_browser_ui_views_something_sources.

Then, for mac_views_browser, we add all the views files and just remove the
cocoa files that would conflict.


Below... lots of nitty comments :). I guess I'm trying to put myself in the
shoes of someone adding a new file and not really having a clue what list to put
it in.

https://codereview.chromium.org/987483003/diff/60001/chrome/chrome_browser_ui...
File chrome/chrome_browser_ui.gypi (right):

https://codereview.chromium.org/987483003/diff/60001/chrome/chrome_browser_ui...
chrome/chrome_browser_ui.gypi:1846: # Cross-platform views sources for all
desktop platforms.
I liked the old comment :). The idea is that when someone adds a file, they will
be steered away from putting it in this list, since it's probably not ready to
run on Mac yet.

https://codereview.chromium.org/987483003/diff/60001/chrome/chrome_browser_ui...
chrome/chrome_browser_ui.gypi:1869: 'chrome_browser_ui_views_browser_sources': [
I think having both chrome_browser_ui_views_sources and
chrome_browser_ui_views_browser_sources is confusing..

The current chrome_browser_ui_views_sources is the list where we eventually want
everything to be, so I think that's the right name there.

But we probably want new files to go into this list. I see that "non_mac"
doesn't really fit since mac_views_browser wants them. Perhaps
chrome_browser_ui_views_stable_sources ? I don't really like that name, but
can't think of something better yet. In the comment, I wouldn't mention Cocoa
("Mac-Cocoa" is basically "MacViews" v1.0 so having two names is also
confusing).

But I would say in the comment something like "You probably want to add new
files here."

And, to make it clear why this exists I think it's also worth having a `TODO(..)
Migrate to chrome_browser_ui_views_sources when the files are ready for release
on Mac`

https://codereview.chromium.org/987483003/diff/60001/chrome/chrome_browser_ui...
chrome/chrome_browser_ui.gypi:2298:
'chrome_browser_ui_views_non_chromeos_sources': [
nit: maybe move this up to line ~1867 for a cleaner diff. (if it really does
belong down here, we can do a follow-up with just the move)

https://codereview.chromium.org/987483003/diff/60001/chrome/chrome_browser_ui...
chrome/chrome_browser_ui.gypi:2314: 'chrome_browser_ui_views_non_mac_sources': [
What's special about this list? E.g. is it just the views stuff that doesn't
compile yet on Mac, or will it never belong on Mac. If the former, there should
be a TODO(..) move to foo. If it's the latter, maybe it can go in _aura_sources.

[Andre, 2015-03-06 18:34:39]

Thanks!

https://codereview.chromium.org/987483003/diff/60001/chrome/chrome_browser_ui...
File chrome/chrome_browser_ui.gypi (right):

https://codereview.chromium.org/987483003/diff/60001/chrome/chrome_browser_ui...
chrome/chrome_browser_ui.gypi:1846: # Cross-platform views sources for all
desktop platforms.
On 2015/03/06 02:38:57, tapted wrote:
> I liked the old comment :). The idea is that when someone adds a file, they
will
> be steered away from putting it in this list, since it's probably not ready to
> run on Mac yet. 

Done.

https://codereview.chromium.org/987483003/diff/60001/chrome/chrome_browser_ui...
chrome/chrome_browser_ui.gypi:1869: 'chrome_browser_ui_views_browser_sources': [
Yeah, I think non_mac is probably the best name for now.
It would be clearest to most people, it's still basically non-mac to everyone
except for us (for now).
Changing it back.

https://codereview.chromium.org/987483003/diff/60001/chrome/chrome_browser_ui...
chrome/chrome_browser_ui.gypi:2298:
'chrome_browser_ui_views_non_chromeos_sources': [
On 2015/03/06 02:38:57, tapted wrote:
> nit: maybe move this up to line ~1867 for a cleaner diff. (if it really does
> belong down here, we can do a follow-up with just the move)

Done.

https://codereview.chromium.org/987483003/diff/60001/chrome/chrome_browser_ui...
chrome/chrome_browser_ui.gypi:2314: 'chrome_browser_ui_views_non_mac_sources': [
On 2015/03/06 02:38:57, tapted wrote:
> What's special about this list? E.g. is it just the views stuff that doesn't
> compile yet on Mac, or will it never belong on Mac. If the former, there
should
> be a TODO(..) move to foo. If it's the latter, maybe it can go in
_aura_sources.

Done, _aura_sources is a good place for them.

[Andre, 2015-03-06 18:36:46]

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

[Andre, 2015-03-06 18:36:46]

sky@, PTAL.
toolkit_views=1 mac_views_browser is close to building.
It should actually build with this change plus 3 more small CLs I have
currently out for review.

[sky, 2015-03-06 19:30:48]

LGTM

[tapted, 2015-03-06 22:47:15]

lgtm

[Andre, 2015-03-06 22:50:52]

The CQ bit was checked by andresantoso@chromium.org

[Andre, 2015-03-06 22:50:53]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org
Link to the patchset: https://codereview.chromium.org/987483003/revert#ps100001
(title: "Update GN")

[commit-bot: I haz the power, 2015-03-06 22:52:16]

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

[commit-bot: I haz the power, 2015-03-07 00:16:31]

Committed patchset #3 (id:100001)

[commit-bot: I haz the power, 2015-03-07 00:17:25]

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

[Nico, 2015-03-24 21:55:10]

thakis@chromium.org changed reviewers:
+ thakis@chromium.org

[Nico, 2015-03-24 21:55:11]

https://codereview.chromium.org/987483003/diff/100001/chrome/chrome_browser_u...
File chrome/chrome_browser_ui.gypi (left):

https://codereview.chromium.org/987483003/diff/100001/chrome/chrome_browser_u...
chrome/chrome_browser_ui.gypi:2979: 'browser/ui/views/task_manager_view.cc',
This isn't good: Now both task_manager_mac and task_manager_view get compiled
in, and both define `chrome::ShowTaskManager(Browser*)`, which gives us an ODR
violation.

[tapted, 2015-03-25 14:01:21]

https://codereview.chromium.org/987483003/diff/100001/chrome/chrome_browser_u...
File chrome/chrome_browser_ui.gypi (left):

https://codereview.chromium.org/987483003/diff/100001/chrome/chrome_browser_u...
chrome/chrome_browser_ui.gypi:2979: 'browser/ui/views/task_manager_view.cc',
On 2015/03/24 21:55:11, Nico wrote:
> This isn't good: Now both task_manager_mac and task_manager_view get compiled
> in, and both define `chrome::ShowTaskManager(Browser*)`, which gives us an ODR
> violation.

Wow you're right - do you have a secret way of detecting this, or are you just
using your thakis superpowers?

I'm amazed this isn't picked up at link time. It looks like it's pruning
task_manager_view.cc completely, but we obviously can't depend on that.

E.g. commenting out ShowTaskManager() in task_manager_mac.mm gives a symbol
collision on Hide now that "something" in task_manager_view.cc is needed.

duplicate symbol __ZN6chrome15HideTaskManagerEv in:
    libbrowser_ui.a(browser_ui.task_manager_mac.o)
    libbrowser_ui.a(browser_ui.task_manager_view.o)

[Nico, 2015-03-25 17:02:59]

On 2015/03/25 14:01:21, tapted (travelling) wrote:
>
https://codereview.chromium.org/987483003/diff/100001/chrome/chrome_browser_u...
> File chrome/chrome_browser_ui.gypi (left):
> 
>
https://codereview.chromium.org/987483003/diff/100001/chrome/chrome_browser_u...
> chrome/chrome_browser_ui.gypi:2979: 'browser/ui/views/task_manager_view.cc',
> On 2015/03/24 21:55:11, Nico wrote:
> > This isn't good: Now both task_manager_mac and task_manager_view get
compiled
> > in, and both define `chrome::ShowTaskManager(Browser*)`, which gives us an
ODR
> > violation.
> 
> Wow you're right - do you have a secret way of detecting this, or are you just
> using your thakis superpowers?
> 
> I'm amazed this isn't picked up at link time. It looks like it's pruning
> task_manager_view.cc completely, but we obviously can't depend on that.

The way static libraries work is that for each unresolved symbol, the linker
loads the .o from the .a that defines that symbol, until everything's defined.
If nothing pulls in both .o files, there's no symbol conflict. (And there's none
during .a construction because dupe symbols are actually pretty common, for
inline functions).
https://plus.google.com/101038813433650812235/posts/fTuNakqTjGh has some more
details on this.

One way to find this is to add -Wl,-all_load to your linker flags. This forces
the linker to load all .o files from .a files. Another way is to have your
target be a source_set in gn. source_sets are just a collection of .o files, not
a .a file, so everything gets loaded. (I once had a gyp patch that turned all
static_libraries effectively into source_sets under the covers; patching that in
would also repro this – but I can't find that right now.) Yet another way is to
try and build chrome with LTO, which is how I came across this.

Anyhoo, please fix :-)

> E.g. commenting out ShowTaskManager() in task_manager_mac.mm gives a symbol
> collision on Hide now that "something" in task_manager_view.cc is needed.
> 
> duplicate symbol __ZN6chrome15HideTaskManagerEv in:
>     libbrowser_ui.a(browser_ui.task_manager_mac.o)
>     libbrowser_ui.a(browser_ui.task_manager_view.o)

[Andre, 2015-03-25 17:41:35]

On 2015/03/25 17:02:59, Nico wrote:
> On 2015/03/25 14:01:21, tapted (travelling) wrote:
> >
>
https://codereview.chromium.org/987483003/diff/100001/chrome/chrome_browser_u...
> > File chrome/chrome_browser_ui.gypi (left):
> > 
> >
>
https://codereview.chromium.org/987483003/diff/100001/chrome/chrome_browser_u...
> > chrome/chrome_browser_ui.gypi:2979: 'browser/ui/views/task_manager_view.cc',
> > On 2015/03/24 21:55:11, Nico wrote:
> > > This isn't good: Now both task_manager_mac and task_manager_view get
> compiled
> > > in, and both define `chrome::ShowTaskManager(Browser*)`, which gives us an
> ODR
> > > violation.
> > 
> > Wow you're right - do you have a secret way of detecting this, or are you
just
> > using your thakis superpowers?
> > 
> > I'm amazed this isn't picked up at link time. It looks like it's pruning
> > task_manager_view.cc completely, but we obviously can't depend on that.
> 
> The way static libraries work is that for each unresolved symbol, the linker
> loads the .o from the .a that defines that symbol, until everything's defined.
> If nothing pulls in both .o files, there's no symbol conflict. (And there's
none
> during .a construction because dupe symbols are actually pretty common, for
> inline functions).
> https://plus.google.com/101038813433650812235/posts/fTuNakqTjGh has some more
> details on this.
> 
> One way to find this is to add -Wl,-all_load to your linker flags. This forces
> the linker to load all .o files from .a files. Another way is to have your
> target be a source_set in gn. source_sets are just a collection of .o files,
not
> a .a file, so everything gets loaded. (I once had a gyp patch that turned all
> static_libraries effectively into source_sets under the covers; patching that
in
> would also repro this – but I can't find that right now.) Yet another way is
to
> try and build chrome with LTO, which is how I came across this.
> 
> Anyhoo, please fix :-)
> 
> > E.g. commenting out ShowTaskManager() in task_manager_mac.mm gives a symbol
> > collision on Hide now that "something" in task_manager_view.cc is needed.
> > 
> > duplicate symbol __ZN6chrome15HideTaskManagerEv in:
> >     libbrowser_ui.a(browser_ui.task_manager_mac.o)
> >     libbrowser_ui.a(browser_ui.task_manager_view.o)

Thanks for catching this, Nico.
I'm working on a fix in http://crrev.com/1003323005.