Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2053563002:
Incognito icon shows up again when opening incognito browser. (Closed)
|
Created:
4 years, 6 months ago by sammiequon Modified:
4 years, 6 months ago CC:
chromium-reviews, tfarina Base URL:
https://chromium.googlesource.com/chromium/src.git@lkgr Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionIncognito icon shows up again when opening incognito browser.
BUG=615360
Committed: https://crrev.com/42de292a4470cd38de8293ca78fe0b0c6561685f
Cr-Commit-Position: refs/heads/master@{#399578}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Refractored code change. #
Total comments: 2
Patch Set 3 : Refactored the change. #Patch Set 4 : Ran git cl format. #Messages
Total messages: 25 (12 generated)
Description was changed from ========== Incognito icon shows up again when opening incognito browser. BUG=615360 ========== to ========== Incognito icon shows up again when opening incognito browser. BUG=615360 ==========
sammiequon@chromium.org changed reviewers: + jdufault@chromium.org
Please take a look.
https://codereview.chromium.org/2053563002/diff/1/chrome/browser/ui/views/fra... File chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc (right): https://codereview.chromium.org/2053563002/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:404: browser_view()->GetNativeWindow())) || is_incognito) { The (((a || b) && c) || d) structure is hard to parse and takes some time to understand. Can you pull the ((a || b) && c) bit into a helper variable?
https://codereview.chromium.org/2053563002/diff/1/chrome/browser/ui/views/fra... File chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc (right): https://codereview.chromium.org/2053563002/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:404: browser_view()->GetNativeWindow())) || is_incognito) { On 2016/06/09 15:54:40, jdufault wrote: > The (((a || b) && c) || d) structure is hard to parse and takes some time to > understand. Can you pull the ((a || b) && c) bit into a helper variable? Done.
lgtm https://codereview.chromium.org/2053563002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc (right): https://codereview.chromium.org/2053563002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:400: const Profile* profile = browser_view()->browser()->profile(); nit: This can be shortened to browser->profile()
The CQ bit was checked by sammiequon@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2053563002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
sammiequon@chromium.org changed reviewers: + pkasting@chromium.org
@pkasting - please take a look
LGTM https://codereview.chromium.org/2053563002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc (right): https://codereview.chromium.org/2053563002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:404: bool is_incognito = profile->GetProfileType() == Profile::INCOGNITO_PROFILE; The incognito case can only apply when the (tabbed || app) check succeeds. So I might write this as: Browser* browser = browser_view()->browser(); if (!browser_->is_type_tabbed() && !browser_->is_app()) return; if ((browser->profile()->GetProfileType() == Profile::INCOGNITO_PROFILE) || chrome::MultiUserWindowManager::ShouldShowAvatar( browser_view()->GetNativeWindow())) UpdateProfileIndicatorIcon();
The CQ bit was checked by sammiequon@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jdufault@chromium.org, pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/2053563002/#ps40001 (title: "Refactored the change.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2053563002/40001
The CQ bit was unchecked by sammiequon@chromium.org
The CQ bit was checked by sammiequon@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jdufault@chromium.org, pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/2053563002/#ps60001 (title: "Ran git cl format.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2053563002/60001
Message was sent while issue was closed.
Description was changed from ========== Incognito icon shows up again when opening incognito browser. BUG=615360 ========== to ========== Incognito icon shows up again when opening incognito browser. BUG=615360 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
CQ bit was unchecked
Message was sent while issue was closed.
Description was changed from ========== Incognito icon shows up again when opening incognito browser. BUG=615360 ========== to ========== Incognito icon shows up again when opening incognito browser. BUG=615360 Committed: https://crrev.com/42de292a4470cd38de8293ca78fe0b0c6561685f Cr-Commit-Position: refs/heads/master@{#399578} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/42de292a4470cd38de8293ca78fe0b0c6561685f Cr-Commit-Position: refs/heads/master@{#399578} |
