MacViews: Get views based toolbar to compile on the Mac.

MacViews: Get views based toolbar to compile on the Mac.

Rename files to avoid duplicate basename conflicts.
Fix missing titlecase strings.

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

[Andre, 2014-10-20 22:32:02]

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

[Andre, 2014-10-20 22:32:02]

Robert, please review.

[Robert Sesek, 2014-10-20 22:33:44]

This seems like kind of a grungy path to go down, since it duplicates naming
naming from cocoa/ and _cocoa. How many instances, including
https://codereview.chromium.org/664223002/, need to be renamed?

[Andre, 2014-10-20 22:40:19]

On 2014/10/20 22:33:44, Robert Sesek wrote:
> This seems like kind of a grungy path to go down, since it duplicates naming
> naming from cocoa/ and _cocoa. How many instances, including
> https://codereview.chromium.org/664223002/, need to be renamed?

Looks like there are 4 more in addition to this, for a total of 8.

static library
/Users/andresantoso/chrome/src/chrome/chrome.gyp:browser_ui#target has several
files with the same basename:
  sad_tab_view: browser/ui/cocoa/tab_contents/sad_tab_view.mm
browser/ui/views/sad_tab_view.cc
  pdf_password_dialog: browser/ui/cocoa/pdf_password_dialog.mm
browser/ui/views/pdf_password_dialog.cc
  bookmark_bar_view: browser/ui/cocoa/bookmarks/bookmark_bar_view.mm
browser/ui/views/bookmarks/bookmark_bar_view.cc
  download_shelf_view: browser/ui/cocoa/download/download_shelf_view.mm
browser/ui/views/download/download_shelf_view.cc

This is the kind of warnings I was getting from libtool:
libtool: warning same member name (browser_ui.bookmark_bar_view.o) in output
file used for input files:
obj/chrome/browser/ui/cocoa/bookmarks/browser_ui.bookmark_bar_view.o and:
obj/chrome/browser/ui/views/bookmarks/browser_ui.bookmark_bar_view.o (due to use
of basename, truncation, blank padding or duplicate input files)

[Robert Sesek, 2014-10-20 22:43:04]

rsesek@chromium.org changed reviewers:
+ mark@chromium.org

[Robert Sesek, 2014-10-20 22:43:05]

On 2014/10/20 22:40:19, Andre wrote:
> On 2014/10/20 22:33:44, Robert Sesek wrote:
> > This seems like kind of a grungy path to go down, since it duplicates naming
> > naming from cocoa/ and _cocoa. How many instances, including
> > https://codereview.chromium.org/664223002/, need to be renamed?
> 
> Looks like there are 4 more in addition to this, for a total of 8.
> 
> static library
> /Users/andresantoso/chrome/src/chrome/chrome.gyp:browser_ui#target has several
> files with the same basename:
>   sad_tab_view: browser/ui/cocoa/tab_contents/sad_tab_view.mm
> browser/ui/views/sad_tab_view.cc
>   pdf_password_dialog: browser/ui/cocoa/pdf_password_dialog.mm
> browser/ui/views/pdf_password_dialog.cc
>   bookmark_bar_view: browser/ui/cocoa/bookmarks/bookmark_bar_view.mm
> browser/ui/views/bookmarks/bookmark_bar_view.cc
>   download_shelf_view: browser/ui/cocoa/download/download_shelf_view.mm
> browser/ui/views/download/download_shelf_view.cc
> 
> This is the kind of warnings I was getting from libtool:
> libtool: warning same member name (browser_ui.bookmark_bar_view.o) in output
> file used for input files:
> obj/chrome/browser/ui/cocoa/bookmarks/browser_ui.bookmark_bar_view.o and:
> obj/chrome/browser/ui/views/bookmarks/browser_ui.bookmark_bar_view.o (due to
use
> of basename, truncation, blank padding or duplicate input files)

Right, I figured this was because of duplicate basenames. I think the only other
way to fix this is to put the duplicate-basenamed files into different targets
:|. If this were more than eight files, I'd say that's probably not worth it.

+mark for his thoughts

[Mark Mentovai, 2014-10-21 03:31:31]

Robert’s right, duplicate basenames can’t coexist in the same target.

Is there a real configuration where we’ll build both cocoa/*.mm and views/*.cc?

[Andre, 2014-10-21 05:41:00]

On 2014/10/21 03:31:31, Mark Mentovai wrote:
> Robert’s right, duplicate basenames can’t coexist in the same target.
> 
> Is there a real configuration where we’ll build both cocoa/*.mm and
views/*.cc?

Yes, for the MacViews effort we plan to build the views code as part of the Mac
chrome target.
Otherwise build regressions will keep happening due to aura dependencies getting
added to views code.
The views browser code will not be part of the final binary because they are not
referenced.
We are considering using views for the Mac port of project Ares (next gen app
launcher).

[Mark Mentovai, 2014-10-21 13:00:21]

Andre wrote:
> The views browser code will not be part of the final binary because they are
not
> referenced.

How can you be sure? It seems somewhat reasonable that a cocoa file and a views
file that aren’t ever expecting to be part of the same build would both wind up
defining things that show up at the same symbol name. Now you’ve got ODR
violations that can manifest themselves in tricky ways, not all of which will
even result in linker errors.

Since these two flavors aren’t supposed to show up in a shipping product
together, I don’t consider this a wise approach.

[Robert Sesek, 2014-10-22 15:47:55]

As an alternative: could you merely filter out the cocoa/ files using gyp
sources rules?

[Mark Mentovai, 2014-10-22 16:41:55]

Andre and I talked about this. We’ll always get object files that have
Objective-C @implementations because we link with -ObjC. The risk is mostly
limited to a views file silently accidentally using a cocoa implementation of
something instead of a views one, but that shouldn’t regress existing cocoa
uses, and the views uses should be hidden behind a flag. It’s not perfect, but
the risks are substantially low enough that I think this is OK to proceed as-is.

[pink (ping after 24hrs), 2014-10-22 16:43:04]

Thanks for working this out, guys!

On Wed, Oct 22, 2014 at 12:41 PM, <mark@chromium.org> wrote:

> Andre and I talked about this. We'll always get object files that have
> Objective-C @implementations because we link with -ObjC. The risk is mostly
> limited to a views file silently accidentally using a cocoa implementation
> of
> something instead of a views one, but that shouldn't regress existing cocoa
> uses, and the views uses should be hidden behind a flag. It's not perfect,
> but
> the risks are substantially low enough that I think this is OK to proceed
> as-is.
>
> https://codereview.chromium.org/657443004/
>



-- 
Mike Pinkerton
Mac Weenie
pinkerton@google.com

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Andre, 2014-10-22 18:35:34]

On 2014/10/22 16:43:04, pink wrote:
> Thanks for working this out, guys!
> 
> On Wed, Oct 22, 2014 at 12:41 PM, <mailto:mark@chromium.org> wrote:
> 
> > Andre and I talked about this. We'll always get object files that have
> > Objective-C @implementations because we link with -ObjC. The risk is mostly
> > limited to a views file silently accidentally using a cocoa implementation
> > of
> > something instead of a views one, but that shouldn't regress existing cocoa
> > uses, and the views uses should be hidden behind a flag. It's not perfect,
> > but
> > the risks are substantially low enough that I think this is OK to proceed
> > as-is.
> >
> > https://codereview.chromium.org/657443004/
> >
> 
> 
> 
> -- 
> Mike Pinkerton
> Mac Weenie
> mailto:pinkerton@google.com
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Robert, LGTY?

[Robert Sesek, 2014-10-22 19:47:09]

LGTM

[Andre, 2014-10-22 21:26:00]

The CQ bit was checked by andresantoso@chromium.org

[commit-bot: I haz the power, 2014-10-22 21:27:47]

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

[commit-bot: I haz the power, 2014-10-22 21:36:54]

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

[commit-bot: I haz the power, 2014-10-22 21:36:56]

Try jobs failed on following builders:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/72255)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/77176)
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
  android_arm64_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)
  android_chromium_gn_compile_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_clang_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...)
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Andre, 2014-10-23 23:12:24]

Patchset #3 (id:40001) has been deleted

[Andre, 2014-10-24 04:09:24]

The CQ bit was checked by andresantoso@chromium.org

[commit-bot: I haz the power, 2014-10-24 04:09:45]

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

[commit-bot: I haz the power, 2014-10-24 05:19:04]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2014-10-24 05:19:57]

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