New issue
Advanced search Search tips

Issue 627301 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Sep 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 3
Type: Bug



Sign in to add a comment

Add output_all_resource_defines=false to chrome/app/theme/theme_resources.grd

Project Member Reported by thestig@chromium.org, Jul 12 2016

Issue description

See  bug 333201  for motivations. Not sure what component to put, so UI>Browser>Themes for now.

1) I had issues with IDR_APP_WINDOW_CLOSE and related resources. chrome/app/theme/theme_resources.grd says <if expr="toolkit_views and not is_macosx"> but apps/ui/views/app_window_frame_view.cc seems to build on all platforms that use Views? Should the .grd file be updated?
2) There may be some failures in: WebUISourcesTest.ThemeSourceImages, UserManagerUIBrowserTest.*, UserManagerBrowserTest.*, ProfileChooserViewExtensionsTest.*, ProfileWindowWebUIBrowserTest.*
3) There is probably a conflict between output_all_resource_defines=false and using grit's if-then-else feature. Avoid using that for now? May be the cause of (2).
 
Summary: Add output_all_resource_defines=false to chrome/app/theme/theme_resources.grd (was: Add output_all_resource_defines=false for chrome/app/theme/theme_resources.grd)

Comment 2 by tapted@chromium.org, Jul 12 2016

For (1), app_window_frame_view.cc can probably be dropped from Mac for now - leave that to me.

The toolkit-views version of the thing that would create an AppWindowFrameView currently always uses a native frame instead. But that's a bug - it probably _should_ be creating a subclass of an AppWindowFrameView, but that's not implemented yet.

views::NonClientFrameView*
ChromeNativeAppWindowViewsMac::CreateNonStandardAppFrame() {
  return new NativeAppWindowFrameViewMac(widget(), this);
}

(haven't looked into (2), (3))
tapted: Does that mean there should be a bug, assigned to you, that blocks this bug? Don't worry about (2) and (3) just yet. (3) especially is just a note to self.

Comment 4 by tapted@chromium.org, Jul 12 2016

 Issue 459877  is the "finish MacViews app windows" bug, but it's not on the roadmap right now. Filed  Issue 627308  for the specific frame thing. I don't think either need to block this bug - we can just drop app_window_frame_view.cc from the mac build for now, and add it back along with IDR_APP_WINDOW_CLOSE when  Issue 627308  gets implemented.
Ok, let's see how https://codereview.chromium.org/2138323002 does on the trybots.
Project Member

Comment 6 by sheriffbot@chromium.org, Jul 12 2017

Labels: Hotlist-Recharge-Cold
Status: Untriaged (was: Available)
This issue has been Available for over a year. If it's no longer important or seems unlikely to be fixed, please consider closing it out. If it is important, please re-triage the issue.

Sorry for the inconvenience if the bug really should have been left as Available. If you change it back, also remove the "Hotlist-Recharge-Cold" label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Labels: -Hotlist-Recharge-Cold
Status: Available (was: Untriaged)
Project Member

Comment 8 by bugdroid1@chromium.org, Sep 15 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/29c2294612b9677d3af3f6eebc6f380b1855c934

commit 29c2294612b9677d3af3f6eebc6f380b1855c934
Author: Evan Stade <estade@chromium.org>
Date: Fri Sep 15 16:29:59 2017

Don't reference Windows specific avatar button resources from Linux.

Bug:  627301 
Change-Id: I6dd293180d393da3fb52647fdd6513991bbf8255
Reviewed-on: https://chromium-review.googlesource.com/668081
Commit-Queue: Evan Stade <estade@chromium.org>
Reviewed-by: Lei Zhang <thestig@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#502278}
[modify] https://crrev.com/29c2294612b9677d3af3f6eebc6f380b1855c934/chrome/browser/ui/views/profiles/avatar_button.cc

Project Member

Comment 9 by bugdroid1@chromium.org, Sep 18 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d7608a0e6700c6b4a8e5fcbf59dc95e889306818

commit d7608a0e6700c6b4a8e5fcbf59dc95e889306818
Author: Lei Zhang <thestig@chromium.org>
Date: Mon Sep 18 22:30:09 2017

Add output_all_resource_defines=false to theme_resources.grd.

BUG= 627301 

Change-Id: I18ed277e587992f8b9a9b7f9ea1507cdfd089f68
Reviewed-on: https://chromium-review.googlesource.com/664311
Reviewed-by: Trent Apted <tapted@chromium.org>
Commit-Queue: Lei Zhang <thestig@chromium.org>
Cr-Commit-Position: refs/heads/master@{#502698}
[modify] https://crrev.com/d7608a0e6700c6b4a8e5fcbf59dc95e889306818/chrome/app/theme/theme_resources.grd
[modify] https://crrev.com/d7608a0e6700c6b4a8e5fcbf59dc95e889306818/chrome/browser/download/download_commands.cc
[modify] https://crrev.com/d7608a0e6700c6b4a8e5fcbf59dc95e889306818/chrome/browser/ui/BUILD.gn

Owner: thestig@chromium.org
Status: Fixed (was: Available)
Thanks for helping folks.

Sign in to add a comment