Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2862113002:
Linux Aura: Use incognito profile to theme incognito browser windows (Closed)
|
Created:
3 years, 7 months ago by Tom (Use chromium acct) Modified:
3 years, 7 months ago CC:
chromium-reviews, tfarina, Evan Stade Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionLinux Aura: Use incognito profile to theme incognito browser windows
This CL sets the theme profile of browser windows to the same as the
actual profile. The theme profile for children windows will continue
using the original (ie. non-incognito) profile.
This fixes an issue where incognito windows using the classic Aura
theme would use NativeThemeAura instead of NativeThemeDarkAura.
BUG=715710
R=sky@chromium.org
CC=estade@chromium.org
Review-Url: https://codereview.chromium.org/2862113002
Cr-Commit-Position: refs/heads/master@{#470478}
Committed: https://chromium.googlesource.com/chromium/src/+/488d6999c101d4a2a6b7cb3ad5b0336d7bb54069
Patch Set 1 #
Total comments: 6
Patch Set 2 : address sky@'s comments #
Total comments: 4
Patch Set 3 : address estade@'s comments #
Total comments: 3
Messages
Total messages: 32 (14 generated)
sky ptal
https://codereview.chromium.org/2862113002/diff/1/chrome/browser/ui/views/fra... File chrome/browser/ui/views/frame/browser_view.cc (right): https://codereview.chromium.org/2862113002/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/browser_view.cc:2089: GetWidget()->SetNativeWindowProperty(Profile::kProfileKey, Could you instead use kProfileKey now? By that I mean remove 2092-2096 and change native_widget_factory.cc to do the same? https://codereview.chromium.org/2862113002/diff/1/chrome/browser/ui/views/nat... File chrome/browser/ui/views/native_widget_factory.cc (right): https://codereview.chromium.org/2862113002/diff/1/chrome/browser/ui/views/nat... chrome/browser/ui/views/native_widget_factory.cc:48: SetThemeProfileForWindow(window, profile->GetOriginalProfile()); I think it's worth a comment that BrowserView resets the value for browser windows to profile_.
https://codereview.chromium.org/2862113002/diff/1/chrome/browser/ui/views/fra... File chrome/browser/ui/views/frame/browser_view.cc (right): https://codereview.chromium.org/2862113002/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/browser_view.cc:2089: GetWidget()->SetNativeWindowProperty(Profile::kProfileKey, On 2017/05/04 23:24:09, sky wrote: > Could you instead use kProfileKey now? By that I mean remove 2092-2096 and > change native_widget_factory.cc to do the same? Can we just make this change: - Profile* GetThemeProfileForWindow(aura::Window* window) { - return window->GetProperty(kThemeProfileKey); - } + Profile* GetThemeProfileForWindow(aura::Window* window) { + Profile* profile = window->GetProperty(kThemeProfileKey); + return profile ? profile : window->GetProperty(kProfileKey); + } chrome_browser_main_extra_parts_views_linux.cc would need to change too if we removed SetThemeProfileForWindow() here
On 2017/05/04 23:41:47, Tom Anderson wrote: > https://codereview.chromium.org/2862113002/diff/1/chrome/browser/ui/views/fra... > File chrome/browser/ui/views/frame/browser_view.cc (right): > > https://codereview.chromium.org/2862113002/diff/1/chrome/browser/ui/views/fra... > chrome/browser/ui/views/frame/browser_view.cc:2089: > GetWidget()->SetNativeWindowProperty(Profile::kProfileKey, > On 2017/05/04 23:24:09, sky wrote: > > Could you instead use kProfileKey now? By that I mean remove 2092-2096 and > > change native_widget_factory.cc to do the same? > > Can we just make this change: > > - Profile* GetThemeProfileForWindow(aura::Window* window) { > - return window->GetProperty(kThemeProfileKey); > - } > + Profile* GetThemeProfileForWindow(aura::Window* window) { > + Profile* profile = window->GetProperty(kThemeProfileKey); > + return profile ? profile : window->GetProperty(kProfileKey); > + } > > chrome_browser_main_extra_parts_views_linux.cc would need to change too if we > removed SetThemeProfileForWindow() here pinging sky
https://codereview.chromium.org/2862113002/diff/1/chrome/browser/ui/views/fra... File chrome/browser/ui/views/frame/browser_view.cc (right): https://codereview.chromium.org/2862113002/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/browser_view.cc:2089: GetWidget()->SetNativeWindowProperty(Profile::kProfileKey, On 2017/05/04 23:41:46, Tom Anderson wrote: > On 2017/05/04 23:24:09, sky wrote: > > Could you instead use kProfileKey now? By that I mean remove 2092-2096 and > > change native_widget_factory.cc to do the same? > > Can we just make this change: > > - Profile* GetThemeProfileForWindow(aura::Window* window) { > - return window->GetProperty(kThemeProfileKey); > - } > + Profile* GetThemeProfileForWindow(aura::Window* window) { > + Profile* profile = window->GetProperty(kThemeProfileKey); > + return profile ? profile : window->GetProperty(kProfileKey); > + } > > chrome_browser_main_extra_parts_views_linux.cc would need to change too if we > removed SetThemeProfileForWindow() here Could you clarify why we need both kThemeProfileKey and kProfileKey? Under what conditions do they differ?
And sorry for the delay in responding.
On 2017/05/08 21:44:38, sky wrote: > https://codereview.chromium.org/2862113002/diff/1/chrome/browser/ui/views/fra... > File chrome/browser/ui/views/frame/browser_view.cc (right): > > https://codereview.chromium.org/2862113002/diff/1/chrome/browser/ui/views/fra... > chrome/browser/ui/views/frame/browser_view.cc:2089: > GetWidget()->SetNativeWindowProperty(Profile::kProfileKey, > On 2017/05/04 23:41:46, Tom Anderson wrote: > > On 2017/05/04 23:24:09, sky wrote: > > > Could you instead use kProfileKey now? By that I mean remove 2092-2096 and > > > change native_widget_factory.cc to do the same? > > > > Can we just make this change: > > > > - Profile* GetThemeProfileForWindow(aura::Window* window) { > > - return window->GetProperty(kThemeProfileKey); > > - } > > + Profile* GetThemeProfileForWindow(aura::Window* window) { > > + Profile* profile = window->GetProperty(kThemeProfileKey); > > + return profile ? profile : window->GetProperty(kProfileKey); > > + } > > > > chrome_browser_main_extra_parts_views_linux.cc would need to change too if we > > removed SetThemeProfileForWindow() here > > Could you clarify why we need both kThemeProfileKey and kProfileKey? Under what > conditions do they differ? They differ when creating children windows from an incognito window. eg. If the task manager is opened from an incognito window, it should be themed with the parent profile (because the incognito profile might go away before the task manager).
Can you give me an example of that? The task manager isn't a browser-window, so it won't have Profile::kProfileKey, but does that matter? Could you set Profile::kProfileKey for the task manager instead of kThemeProfileKey? On Mon, May 8, 2017 at 2:46 PM, thomasanderson via Chromium-reviews < chromium-reviews@chromium.org> wrote: > On 2017/05/08 21:44:38, sky wrote: > > > https://codereview.chromium.org/2862113002/diff/1/chrome/ > browser/ui/views/frame/browser_view.cc > > File chrome/browser/ui/views/frame/browser_view.cc (right): > > > > > https://codereview.chromium.org/2862113002/diff/1/chrome/ > browser/ui/views/frame/browser_view.cc#newcode2089 > > chrome/browser/ui/views/frame/browser_view.cc:2089: > > GetWidget()->SetNativeWindowProperty(Profile::kProfileKey, > > On 2017/05/04 23:41:46, Tom Anderson wrote: > > > On 2017/05/04 23:24:09, sky wrote: > > > > Could you instead use kProfileKey now? By that I mean remove > 2092-2096 and > > > > change native_widget_factory.cc to do the same? > > > > > > Can we just make this change: > > > > > > - Profile* GetThemeProfileForWindow(aura::Window* window) { > > > - return window->GetProperty(kThemeProfileKey); > > > - } > > > + Profile* GetThemeProfileForWindow(aura::Window* window) { > > > + Profile* profile = window->GetProperty(kThemeProfileKey); > > > + return profile ? profile : window->GetProperty(kProfileKey); > > > + } > > > > > > chrome_browser_main_extra_parts_views_linux.cc would need to change > too if > we > > > removed SetThemeProfileForWindow() here > > > > Could you clarify why we need both kThemeProfileKey and kProfileKey? > Under > what > > conditions do they differ? > > They differ when creating children windows from an incognito window. eg. > If the > task manager is opened from an incognito window, it should be themed with > the > parent profile (because the incognito profile might go away before the task > manager). > > https://codereview.chromium.org/2862113002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2017/05/08 22:40:55, sky wrote: > Can you give me an example of that? The task manager isn't a > browser-window, so it won't have Profile::kProfileKey, but does that > matter? Could you set Profile::kProfileKey for the task manager instead > of kThemeProfileKey? You're right, we can eliminate kThemeProfileKey. This is done in the latest patch set. I can't help but feel like I've already written this code before ;) https://codereview.chromium.org/2862113002/diff/1/chrome/browser/ui/views/fra... File chrome/browser/ui/views/frame/browser_view.cc (right): https://codereview.chromium.org/2862113002/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/browser_view.cc:2089: GetWidget()->SetNativeWindowProperty(Profile::kProfileKey, On 2017/05/08 21:44:37, sky wrote: > On 2017/05/04 23:41:46, Tom Anderson wrote: > > On 2017/05/04 23:24:09, sky wrote: > > > Could you instead use kProfileKey now? By that I mean remove 2092-2096 and > > > change native_widget_factory.cc to do the same? > > > > Can we just make this change: > > > > - Profile* GetThemeProfileForWindow(aura::Window* window) { > > - return window->GetProperty(kThemeProfileKey); > > - } > > + Profile* GetThemeProfileForWindow(aura::Window* window) { > > + Profile* profile = window->GetProperty(kThemeProfileKey); > > + return profile ? profile : window->GetProperty(kProfileKey); > > + } > > > > chrome_browser_main_extra_parts_views_linux.cc would need to change too if we > > removed SetThemeProfileForWindow() here > > Could you clarify why we need both kThemeProfileKey and kProfileKey? Under what > conditions do they differ? Nevermind, I see what you mean now. I think we can just use kProfileKey only. https://codereview.chromium.org/2862113002/diff/1/chrome/browser/ui/views/nat... File chrome/browser/ui/views/native_widget_factory.cc (right): https://codereview.chromium.org/2862113002/diff/1/chrome/browser/ui/views/nat... chrome/browser/ui/views/native_widget_factory.cc:48: SetThemeProfileForWindow(window, profile->GetOriginalProfile()); On 2017/05/04 23:24:09, sky wrote: > I think it's worth a comment that BrowserView resets the value for browser > windows to profile_. Done.
The CQ bit was checked by thomasanderson@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
estade@chromium.org changed reviewers: + estade@chromium.org
please excuse the drive-by https://codereview.chromium.org/2862113002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/native_widget_factory.cc (right): https://codereview.chromium.org/2862113002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/native_widget_factory.cc:42: } else { nit: no else after return https://codereview.chromium.org/2862113002/diff/20001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/desktop_native_widget_aura.h (right): https://codereview.chromium.org/2862113002/diff/20001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_native_widget_aura.h:103: // Overridden from internal::NativeWidgetPrivate: nit: there's already a block for NativeWidgetPrivate overrides just above this line.
https://codereview.chromium.org/2862113002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/native_widget_factory.cc (right): https://codereview.chromium.org/2862113002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/native_widget_factory.cc:42: } else { On 2017/05/09 15:43:26, Evan Stade wrote: > nit: no else after return Done. https://codereview.chromium.org/2862113002/diff/20001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/desktop_native_widget_aura.h (right): https://codereview.chromium.org/2862113002/diff/20001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_native_widget_aura.h:103: // Overridden from internal::NativeWidgetPrivate: On 2017/05/09 15:43:26, Evan Stade wrote: > nit: there's already a block for NativeWidgetPrivate overrides just above this > line. Done.
The CQ bit was checked by thomasanderson@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM https://codereview.chromium.org/2862113002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/native_widget_factory.cc (right): https://codereview.chromium.org/2862113002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/native_widget_factory.cc:32: // However, the original profile will stick around until shutdown. I think it's worth a comment that BrowserView resets this value later on to the real profile.
https://codereview.chromium.org/2862113002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/native_widget_factory.cc (right): https://codereview.chromium.org/2862113002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/native_widget_factory.cc:32: // However, the original profile will stick around until shutdown. On 2017/05/09 16:58:24, sky wrote: > I think it's worth a comment that BrowserView resets this value later on to the > real profile. Already done. See the comment above this one
https://codereview.chromium.org/2862113002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/native_widget_factory.cc (right): https://codereview.chromium.org/2862113002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/native_widget_factory.cc:32: // However, the original profile will stick around until shutdown. On 2017/05/09 17:24:09, Tom Anderson wrote: > On 2017/05/09 16:58:24, sky wrote: > > I think it's worth a comment that BrowserView resets this value later on to > the > > real profile. > > Already done. See the comment above this one Sorry. I some how missed that.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by thomasanderson@google.com
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?))
The CQ bit was checked by thomasanderson@google.com
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1494393863894510,
"parent_rev": "d69d277c17e3a2a36b281ae887fe1d5a4de7a9d4", "commit_rev":
"488d6999c101d4a2a6b7cb3ad5b0336d7bb54069"}
Message was sent while issue was closed.
Description was changed from ========== Linux Aura: Use incognito profile to theme incognito browser windows This CL sets the theme profile of browser windows to the same as the actual profile. The theme profile for children windows will continue using the original (ie. non-incognito) profile. This fixes an issue where incognito windows using the classic Aura theme would use NativeThemeAura instead of NativeThemeDarkAura. BUG=715710 R=sky@chromium.org CC=estade@chromium.org ========== to ========== Linux Aura: Use incognito profile to theme incognito browser windows This CL sets the theme profile of browser windows to the same as the actual profile. The theme profile for children windows will continue using the original (ie. non-incognito) profile. This fixes an issue where incognito windows using the classic Aura theme would use NativeThemeAura instead of NativeThemeDarkAura. BUG=715710 R=sky@chromium.org CC=estade@chromium.org Review-Url: https://codereview.chromium.org/2862113002 Cr-Commit-Position: refs/heads/master@{#470478} Committed: https://chromium.googlesource.com/chromium/src/+/488d6999c101d4a2a6b7cb3ad5b0... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/488d6999c101d4a2a6b7cb3ad5b0... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
