Revert r123782.

Revert r123782.

See bug for why.

BUG=108554
TEST=none
TBR=ben

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=135063

[Nico, 2012-05-02 22:34:04]

+tfarina
On May 2, 2012 2:59 PM, <thakis@chromium.org> wrote:

> Reviewers: Elliot Glaysher,
>
> Description:
> Revert r123782.
>
> See bug for why.
>
> BUG=108554
> TEST=none
>
>
> Please review this at
https://chromiumcodereview.**appspot.com/10337010/<https://chromiumcodereview...
>
> SVN Base:
svn://svn.chromium.org/chrome/**trunk/src<http://svn.chromium.org/chrome/trunk/src>
>
> Affected files:
>  M chrome/browser/renderer_**preferences_util.cc
>  M chrome/browser/themes/theme_**service_factory.cc
>  M chrome/browser/ui/gtk/avatar_**menu_bubble_gtk.h
>  M chrome/browser/ui/gtk/avatar_**menu_bubble_gtk.cc
>  M chrome/browser/ui/gtk/avatar_**menu_item_gtk.h
>  M chrome/browser/ui/gtk/avatar_**menu_item_gtk.cc
>  M chrome/browser/ui/gtk/back_**forward_button_gtk.cc
>  M chrome/browser/ui/gtk/**bookmarks/bookmark_bar_gtk.h
>  M chrome/browser/ui/gtk/**bookmarks/bookmark_bar_gtk.cc
>  M chrome/browser/ui/gtk/**bookmarks/bookmark_bar_**instructions_gtk.h
>  M chrome/browser/ui/gtk/**bookmarks/bookmark_bar_**instructions_gtk.cc
>  M chrome/browser/ui/gtk/**bookmarks/bookmark_bubble_gtk.**h
>  M chrome/browser/ui/gtk/**bookmarks/bookmark_bubble_gtk.**cc
>  M chrome/browser/ui/gtk/**bookmarks/bookmark_editor_gtk.**cc
>  M chrome/browser/ui/gtk/**bookmarks/bookmark_menu_**controller_gtk.cc
>  M chrome/browser/ui/gtk/**bookmarks/bookmark_tree_model.**cc
>  M chrome/browser/ui/gtk/**bookmarks/bookmark_utils_gtk.h
>  M chrome/browser/ui/gtk/**bookmarks/bookmark_utils_gtk.**cc
>  M chrome/browser/ui/gtk/browser_**actions_toolbar_gtk.h
>  M chrome/browser/ui/gtk/browser_**actions_toolbar_gtk.cc
>  M chrome/browser/ui/gtk/browser_**titlebar.h
>  M chrome/browser/ui/gtk/browser_**titlebar.cc
>  M chrome/browser/ui/gtk/browser_**toolbar_gtk.h
>  M chrome/browser/ui/gtk/browser_**toolbar_gtk.cc
>  M chrome/browser/ui/gtk/browser_**window_gtk.cc
>  M chrome/browser/ui/gtk/bubble/**bubble_gtk.h
>  M chrome/browser/ui/gtk/bubble/**bubble_gtk.cc
>  M chrome/browser/ui/gtk/bubble/**bubble_gtk_browsertest.cc
>  M chrome/browser/ui/gtk/confirm_**bubble_view.cc
>  M chrome/browser/ui/gtk/content_**setting_bubble_gtk.cc
>  M chrome/browser/ui/gtk/custom_**button.h
>  M chrome/browser/ui/gtk/custom_**button.cc
>  M chrome/browser/ui/gtk/**download/download_item_gtk.h
>  M chrome/browser/ui/gtk/**download/download_item_gtk.cc
>  M chrome/browser/ui/gtk/**download/download_shelf_gtk.h
>  M chrome/browser/ui/gtk/**download/download_shelf_gtk.cc
>  M chrome/browser/ui/gtk/**extensions/bundle_installed_**bubble_gtk.cc
>  M chrome/browser/ui/gtk/**extensions/extension_**installed_bubble_gtk.cc
>  M chrome/browser/ui/gtk/**extensions/extension_popup_**gtk.cc
>  M chrome/browser/ui/gtk/find_**bar_gtk.h
>  M chrome/browser/ui/gtk/find_**bar_gtk.cc
>  M chrome/browser/ui/gtk/first_**run_bubble.cc
>  M chrome/browser/ui/gtk/global_**error_bubble.cc
>  M chrome/browser/ui/gtk/global_**history_menu.cc
>  M chrome/browser/ui/gtk/global_**menu_bar.cc
>  A + chrome/browser/ui/gtk/gtk_**theme_service.h
>  A + chrome/browser/ui/gtk/gtk_**theme_service.cc
>  A + chrome/browser/ui/gtk/gtk_**theme_service_unittest.cc
>  M chrome/browser/ui/gtk/gtk_**tree.cc
>  M chrome/browser/ui/gtk/gtk_**util.h
>  M chrome/browser/ui/gtk/gtk_**util.cc
>  M chrome/browser/ui/gtk/**infobars/infobar_gtk.h
>  M chrome/browser/ui/gtk/**infobars/infobar_gtk.cc
>  M chrome/browser/ui/gtk/**location_bar_view_gtk.h
>  M chrome/browser/ui/gtk/**location_bar_view_gtk.cc
>  M chrome/browser/ui/gtk/**notifications/balloon_view_**gtk.h
>  M chrome/browser/ui/gtk/**notifications/balloon_view_**gtk.cc
>  M chrome/browser/ui/gtk/omnibox/**omnibox_popup_view_gtk.h
>  M chrome/browser/ui/gtk/omnibox/**omnibox_popup_view_gtk.cc
>  M chrome/browser/ui/gtk/omnibox/**omnibox_view_gtk.h
>  M chrome/browser/ui/gtk/omnibox/**omnibox_view_gtk.cc
>  M chrome/browser/ui/gtk/**overflow_button.cc
>  M chrome/browser/ui/gtk/page_**info_bubble_gtk.cc
>  M chrome/browser/ui/gtk/reload_**button_gtk.h
>  M chrome/browser/ui/gtk/reload_**button_gtk.cc
>  M chrome/browser/ui/gtk/status_**bubble_gtk.h
>  M chrome/browser/ui/gtk/status_**bubble_gtk.cc
>  M chrome/browser/ui/gtk/tabs/**dragged_view_gtk.cc
>  M chrome/browser/ui/gtk/tabs/**tab_gtk.h
>  M chrome/browser/ui/gtk/tabs/**tab_renderer_gtk.h
>  M chrome/browser/ui/gtk/tabs/**tab_renderer_gtk.cc
>  M chrome/browser/ui/gtk/tabs/**tab_strip_gtk.h
>  M chrome/browser/ui/gtk/tabs/**tab_strip_gtk.cc
>  M chrome/browser/ui/gtk/task_**manager_gtk.cc
>  D chrome/browser/ui/gtk/theme_**service_gtk.h
>  D chrome/browser/ui/gtk/theme_**service_gtk.cc
>  D chrome/browser/ui/gtk/theme_**service_gtk_unittest.cc
>  M chrome/browser/ui/gtk/web_**intent_picker_gtk.cc
>  M chrome/browser/ui/webui/**options2/browser_options_**handler2.cc
>  M chrome/chrome_browser.gypi
>  M chrome/chrome_tests.gypi
>  M ui/gfx/image/cairo_cached_**surface.h
>
>
>

[tfarina, 2012-05-02 22:39:18]

On 2012/05/02 22:34:04, Nico wrote:
> +tfarina
I'm against, and I give my point in the bug (and that will just make me sad, but
I'll try to not bother too much). My complain isn't going to matter much as I'm
not in the OWNERS list of gtk, but Elliot was OK with the reason I gave when I
did it. 

Now is up to him to decide if he is fine in reverting or not. I bet he will be
fine :)

[Elliot Glaysher, 2012-05-02 22:42:18]

lgtm.

Getting a check to make sure there aren't duplicated basenames is really high
priority because it means that on Windows, you get link failures since the
output of one compile clobbers the output of another.

(I was bitten by this a month ago. In addition, it screws up any navigate by
path scripts since basenames aren't unique.)

[tfarina, 2012-05-02 22:42:20]

On 2012/05/02 22:39:18, tfarina wrote:
> On 2012/05/02 22:34:04, Nico wrote:
> > +tfarina
> I'm against, and I give my point in the bug (and that will just make me sad,
but
> I'll try to not bother too much). My complain isn't going to matter much as
I'm
> not in the OWNERS list of gtk, but Elliot was OK with the reason I gave when I
> did it. 
> 
> Now is up to him to decide if he is fine in reverting or not. I bet he will be
> fine :)

It is TOO inconsistent with the naming scheme for my taste. Just look at the
files in this CL and you see _gtk.* not gtk_*

[Nico, 2012-05-02 22:56:13]

On 2012/05/02 22:42:20, tfarina wrote:
> On 2012/05/02 22:39:18, tfarina wrote:
> > On 2012/05/02 22:34:04, Nico wrote:
> > > +tfarina
> > I'm against, and I give my point in the bug (and that will just make me sad,
> but
> > I'll try to not bother too much). My complain isn't going to matter much as
> I'm
> > not in the OWNERS list of gtk, but Elliot was OK with the reason I gave when
I
> > did it. 
> > 
> > Now is up to him to decide if he is fine in reverting or not. I bet he will
be
> > fine :)
> 
> It is TOO inconsistent with the naming scheme for my taste. Just look at the
> files in this CL and you see _gtk.* not gtk_*

hummer:src thakis$ ls -l chrome/browser/ui/gtk/gtk* | wc -l
      26
hummer:src thakis$ ls -l chrome/browser/ui/gtk/*_gtk.* | wc -l
      99


It's a bit inconsistent, but I can't think of a better naming that doesn't have
the 'clashing basename' problem.

[commit-bot: I haz the power, 2012-05-02 22:56:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thakis@chromium.org/10337010/4001

[commit-bot: I haz the power, 2012-05-02 22:57:23]

Presubmit check for 10337010-4001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/browser/ui/webui/options2
    ui/gfx/image

Presubmit checks took 7.2s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[Nico, 2012-05-02 22:59:22]

ben: OWNERS

[commit-bot: I haz the power, 2012-05-02 23:02:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thakis@chromium.org/10337010/4001

[tfarina, 2012-05-02 23:04:04]

On 2012/05/02 22:56:13, Nico wrote:
> On 2012/05/02 22:42:20, tfarina wrote:
> > On 2012/05/02 22:39:18, tfarina wrote:
> > > On 2012/05/02 22:34:04, Nico wrote:
> > > > +tfarina
> > > I'm against, and I give my point in the bug (and that will just make me
sad,
> > but
> > > I'll try to not bother too much). My complain isn't going to matter much
as
> > I'm
> > > not in the OWNERS list of gtk, but Elliot was OK with the reason I gave
when
> I
> > > did it. 
> > > 
> > > Now is up to him to decide if he is fine in reverting or not. I bet he
will
> be
> > > fine :)
> > 
> > It is TOO inconsistent with the naming scheme for my taste. Just look at the
> > files in this CL and you see _gtk.* not gtk_*
> 
> hummer:src thakis$ ls -l chrome/browser/ui/gtk/gtk* | wc -l
>       26
> hummer:src thakis$ ls -l chrome/browser/ui/gtk/*_gtk.* | wc -l
>       99
> 
> 
> It's a bit inconsistent, but I can't think of a better naming that doesn't
have
> the 'clashing basename' problem.

I don't see the problem yet. What error are you getting (i.e, can you past the
MSVC output)? By basename what you mean? theme_service?

Why don't you file a BUG for this task? That way would be much easier to see the
reason behind it.

[Nico, 2012-05-02 23:10:15]

On 2012/05/02 23:04:04, tfarina wrote:
> On 2012/05/02 22:56:13, Nico wrote:
> > On 2012/05/02 22:42:20, tfarina wrote:
> > > On 2012/05/02 22:39:18, tfarina wrote:
> > > > On 2012/05/02 22:34:04, Nico wrote:
> > > > > +tfarina
> > > > I'm against, and I give my point in the bug (and that will just make me
> sad,
> > > but
> > > > I'll try to not bother too much). My complain isn't going to matter much
> as
> > > I'm
> > > > not in the OWNERS list of gtk, but Elliot was OK with the reason I gave
> when
> > I
> > > > did it. 
> > > > 
> > > > Now is up to him to decide if he is fine in reverting or not. I bet he
> will
> > be
> > > > fine :)
> > > 
> > > It is TOO inconsistent with the naming scheme for my taste. Just look at
the
> > > files in this CL and you see _gtk.* not gtk_*
> > 
> > hummer:src thakis$ ls -l chrome/browser/ui/gtk/gtk* | wc -l
> >       26
> > hummer:src thakis$ ls -l chrome/browser/ui/gtk/*_gtk.* | wc -l
> >       99
> > 
> > 
> > It's a bit inconsistent, but I can't think of a better naming that doesn't
> have
> > the 'clashing basename' problem.
> 
> I don't see the problem yet. What error are you getting (i.e, can you past the
> MSVC output)? By basename what you mean? theme_service?
> 
> Why don't you file a BUG for this task? That way would be much easier to see
the
> reason behind it.

I left a comment on the bug in the BUG line.

[tfarina, 2012-05-02 23:11:35]

On 2012/05/02 22:56:13, Nico wrote:
> hummer:src thakis$ ls -l chrome/browser/ui/gtk/gtk* | wc -l
>       26
The files prefixed with gtk_ are so with a reason, they are written in GTK
object oriented style, in plain C, they are not C++ classes.

[tfarina, 2012-05-02 23:20:11]

On Wed, May 2, 2012 at 8:10 PM,  <thakis@chromium.org> wrote:
> On 2012/05/02 23:04:04, tfarina wrote:
>>
>> On 2012/05/02 22:56:13, Nico wrote:
>> > On 2012/05/02 22:42:20, tfarina wrote:
>> > > On 2012/05/02 22:39:18, tfarina wrote:
>> > > > On 2012/05/02 22:34:04, Nico wrote:
>> > > > > +tfarina
>> > > > I'm against, and I give my point in the bug (and that will just make
>> > > > me
>> sad,
>> > > but
>> > > > I'll try to not bother too much). My complain isn't going to matter
>> > > > much
>> as
>> > > I'm
>> > > > not in the OWNERS list of gtk, but Elliot was OK with the reason I
>> > > > gave
>> when
>> > I
>> > > > did it.
>> > > >
>> > > > Now is up to him to decide if he is fine in reverting or not. I bet
>> > > > he
>> will
>> > be
>> > > > fine :)
>> > >
>> > > It is TOO inconsistent with the naming scheme for my taste. Just look
>> > > at
>
> the
>>
>> > > files in this CL and you see _gtk.* not gtk_*
>> >
>> > hummer:src thakis$ ls -l chrome/browser/ui/gtk/gtk* | wc -l
>> >       26
>> > hummer:src thakis$ ls -l chrome/browser/ui/gtk/*_gtk.* | wc -l
>> >       99
>> >
>> >
>> > It's a bit inconsistent, but I can't think of a better naming that
>> > doesn't
>> have
>> > the 'clashing basename' problem.
>
>
>> I don't see the problem yet. What error are you getting (i.e, can you past
>> the
>> MSVC output)? By basename what you mean? theme_service?
>
>
>> Why don't you file a BUG for this task? That way would be much easier to
>> see
>
> the
>>
>> reason behind it.
>
>
> I left a comment on the bug in the BUG line.
>
Thanks! Now I see the problem.

There are chrome/browser/themes/theme_service_gtk.cc and
chrome/browser/ui/gtk/theme_service_gtk.cc. The former implements the
gtk parts of ThemeService, while the later inherits from it.

LGTM!

Although, there would be a better abstraction on top of ThemeService
to avoid this kind of this situation in the first place.

-- 
Thiago

[commit-bot: I haz the power, 2012-05-02 23:38:43]

Try job failure for 10337010-4001 (retry) on linux_clang for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...

[commit-bot: I haz the power, 2012-05-03 00:19:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thakis@chromium.org/10337010/25001

[commit-bot: I haz the power, 2012-05-03 00:24:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thakis@chromium.org/10337010/21002