Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1313383006:
Don't compile save_password_infobar_delegate.cc everywhere but Mac and Android (Closed)
|
Created:
5 years, 3 months ago by ki.stfu Modified:
5 years, 3 months ago CC:
chromium-reviews, tfarina, gcasto+watchlist_chromium.org, vabr+watchlist_chromium.org, mkwst+watchlist-passwords_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDon't compile save_password_infobar_delegate.cc everywhere but Mac and Android
The SavePasswordInfoBarDelegate is used on Mac and Android only. There
is no reason to compile save_password_infobar_delegate.cc on other
platform.
In addition, this patch excludes the k{Enable,Disable}SavePasswordBubble
definitions on Aura platforms.
BUG=526837
TEST=
R=vabr@chromium.org,pkasting@chromium.org,tapted@chromium.org,sky@chromium.org
Committed: https://crrev.com/5e669f04cbfb8655af0afe5854def2263018e1be
Cr-Commit-Position: refs/heads/master@{#349370}
Patch Set 1 #
Total comments: 11
Patch Set 2 : Refactor IsTheHotNewBubbleUIEnabled; Don't use LOG() #
Total comments: 3
Patch Set 3 : Fix a comment; Use NOTREACHED() instead of DCHECK() #Patch Set 4 : Exclude kEnableSavePasswordBubble/kDisableSavePasswordBubble definitions on Aura platforms #
Total comments: 11
Patch Set 5 : Make a bit refactoring #
Total comments: 7
Patch Set 6 : Restore chrome/browser/ui/views/infobars/save_password_infobar.cc #Patch Set 7 : Restore chrome/browser/ui/views/infobars/save_password_infobar.cc #Patch Set 8 : Fix GN build #Patch Set 9 : Rebase against ToT #
Total comments: 2
Patch Set 10 : Add TODO in chrome/browser/ui/views/infobars/save_password_infobar.cc #Patch Set 11 : Rebase against ToT #Patch Set 12 : Rebase against ToT #
Messages
Total messages: 65 (23 generated)
https://codereview.chromium.org/1313383006/diff/1/chrome/browser/password_man... File chrome/browser/password_manager/chrome_password_manager_client.cc (right): https://codereview.chromium.org/1313383006/diff/1/chrome/browser/password_man... chrome/browser/password_manager/chrome_password_manager_client.cc:232: LOG(FATAL) << "New bubble UI should be used everywhere but Mac and Android"; Avoid LOG statements if possible; prefer to do nothing, or if you really need to, NOTREACHED(). https://codereview.chromium.org/1313383006/diff/1/chrome/browser/password_man... chrome/browser/password_manager/chrome_password_manager_client.cc:523: LOG(ERROR) << "--" << switches::kDisableSavePasswordBubble Same comment. https://codereview.chromium.org/1313383006/diff/1/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/1313383006/diff/1/chrome/chrome_browser.gypi#... chrome/chrome_browser.gypi:2051: # Used everywhere but Mac and Android. I'm not sure this comment is accurate. Do we really use none of this on these platforms? In particular, files named XXX_mac.* look Mac-specific.
https://codereview.chromium.org/1313383006/diff/1/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/1313383006/diff/1/chrome/chrome_browser.gypi#... chrome/chrome_browser.gypi:2051: # Used everywhere but Mac and Android. On 2015/09/08 08:20:00, Peter Kasting wrote: > I'm not sure this comment is accurate. Do we really use none of this on these > platforms? In particular, files named XXX_mac.* look Mac-specific. Oops. It's a typo. I'll remove this comment at all.
First of all, thanks for joining Chromium contributors, and writing this CL! :) For the record, I followed https://www.chromium.org/developers/contributing-code/external-contributor-ch... and checked that you signed the CLA. In addition to Peter's comments, I left one more. Also please, check chrome/browser/password_manager/password_manager_test_base.cc -- that file also uses infobar code, which should be ifdefed in the absence of the infobar. Thanks, Vaclav https://codereview.chromium.org/1313383006/diff/1/chrome/browser/password_man... File chrome/browser/password_manager/chrome_password_manager_client.cc (right): https://codereview.chromium.org/1313383006/diff/1/chrome/browser/password_man... chrome/browser/password_manager/chrome_password_manager_client.cc:518: #endif The current changes in this method are not ideal, because 1. There is no reason to offer the command-line flags for the bubble if we don't want to see it (bonus points if you exclude its definition on Aura platforms as well). 2. The corresponding field trial is expired (sorry, I should have mentioned it to you, you have no way to check that), so no need to worry about that. Also, the negations in the ifdefs are getting hard to read. So I suggest the following structure of the code: #if defined(OS_MACOSX) || defined(OS_ANDROID) ... (the current checks of flags and field trials) #elif defined(USE_AURA) return true; #else return false; #endif
estade@chromium.org changed reviewers: + estade@chromium.org
does this break the mac views build?
On 2015/09/08 12:52:50, vabr (Chromium) wrote: > First of all, thanks for joining Chromium contributors, and writing this CL! :) > > For the record, I followed > https://www.chromium.org/developers/contributing-code/external-contributor-ch... > and checked that you signed the CLA. > > In addition to Peter's comments, I left one more. > > Also please, check chrome/browser/password_manager/password_manager_test_base.cc > -- that file also uses infobar code, which should be ifdefed in the absence of > the infobar. > > Thanks, > Vaclav > > https://codereview.chromium.org/1313383006/diff/1/chrome/browser/password_man... > File chrome/browser/password_manager/chrome_password_manager_client.cc (right): > > https://codereview.chromium.org/1313383006/diff/1/chrome/browser/password_man... > chrome/browser/password_manager/chrome_password_manager_client.cc:518: #endif > The current changes in this method are not ideal, because > 1. There is no reason to offer the command-line flags for the bubble if we don't > want to see it (bonus points if you exclude its definition on Aura platforms as > well). > 2. The corresponding field trial is expired (sorry, I should have mentioned it > to you, you have no way to check that), so no need to worry about that. > > Also, the negations in the ifdefs are getting hard to read. So I suggest the > following structure of the code: > > #if defined(OS_MACOSX) || defined(OS_ANDROID) > ... (the current checks of flags and field trials) > #elif defined(USE_AURA) > return true; > #else > return false; > #endif I grep'ed all files and found nothing related to that infobar (except Mac and Android platform-specific code).
https://codereview.chromium.org/1313383006/diff/1/chrome/browser/password_man... File chrome/browser/password_manager/chrome_password_manager_client.cc (right): https://codereview.chromium.org/1313383006/diff/1/chrome/browser/password_man... chrome/browser/password_manager/chrome_password_manager_client.cc:232: LOG(FATAL) << "New bubble UI should be used everywhere but Mac and Android"; On 2015/09/08 08:20:00, Peter Kasting wrote: > Avoid LOG statements if possible; prefer to do nothing, or if you really need > to, NOTREACHED(). I worry about the case if IsTheHotNewBubbleUIEnabled() returns false on Mac or Android. Can I do as follows? ``` const bool is_the_hot_new_bubble_ui_enabled = IsTheHotNewBubbleUIEnabled(); #if !defined(OS_MACOSX) && !defined(OS_ANDROID) DCHECK(is_the_hot_new_bubble_ui_enabled) << "New bubble UI should be used" << " everywhere but Mac and Android platforms"; #endif if (is_the_hot_new_bubble_ui_enabled) { ``` https://codereview.chromium.org/1313383006/diff/1/chrome/browser/password_man... chrome/browser/password_manager/chrome_password_manager_client.cc:518: #endif On 2015/09/08 12:52:49, vabr (Chromium) wrote: > The current changes in this method are not ideal, because > 1. There is no reason to offer the command-line flags for the bubble if we don't > want to see it (bonus points if you exclude its definition on Aura platforms as > well). > 2. The corresponding field trial is expired (sorry, I should have mentioned it > to you, you have no way to check that), so no need to worry about that. > > Also, the negations in the ifdefs are getting hard to read. So I suggest the > following structure of the code: > > #if defined(OS_MACOSX) || defined(OS_ANDROID) > ... (the current checks of flags and field trials) > #elif defined(USE_AURA) > return true; > #else > return false; > #endif > I worry about the last case: ``` #else return false; #endif ``` FMPOV, we should return "true" there, because "false" is only suitable for Mac and Android platforms. https://codereview.chromium.org/1313383006/diff/1/chrome/browser/password_man... chrome/browser/password_manager/chrome_password_manager_client.cc:523: LOG(ERROR) << "--" << switches::kDisableSavePasswordBubble On 2015/09/08 08:20:00, Peter Kasting wrote: > Same comment. Done. https://codereview.chromium.org/1313383006/diff/1/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/1313383006/diff/1/chrome/chrome_browser.gypi#... chrome/chrome_browser.gypi:2051: # Used everywhere but Mac and Android. On 2015/09/08 08:20:00, Peter Kasting wrote: > I'm not sure this comment is accurate. Do we really use none of this on these > platforms? In particular, files named XXX_mac.* look Mac-specific. Done.
On 2015/09/08 19:20:48, Evan Stade wrote: > does this break the mac views build? I can't test it on Mac. Can somebody start a build on buildbot to check whether it breaks Mac build or not?
https://codereview.chromium.org/1313383006/diff/1/chrome/browser/password_man... File chrome/browser/password_manager/chrome_password_manager_client.cc (right): https://codereview.chromium.org/1313383006/diff/1/chrome/browser/password_man... chrome/browser/password_manager/chrome_password_manager_client.cc:232: LOG(FATAL) << "New bubble UI should be used everywhere but Mac and Android"; On 2015/09/08 20:39:54, ki.stfu wrote: > On 2015/09/08 08:20:00, Peter Kasting wrote: > > Avoid LOG statements if possible; prefer to do nothing, or if you really need > > to, NOTREACHED(). > > I worry about the case if IsTheHotNewBubbleUIEnabled() returns false on Mac or > Android. Can I do as follows? > ``` > const bool is_the_hot_new_bubble_ui_enabled = IsTheHotNewBubbleUIEnabled(); > #if !defined(OS_MACOSX) && !defined(OS_ANDROID) > DCHECK(is_the_hot_new_bubble_ui_enabled) << "New bubble UI should be used" > << " everywhere but Mac and Android platforms"; > #endif > > if (is_the_hot_new_bubble_ui_enabled) { > ``` If you're trying to not compile SavePasswordInfoBarDelegate, you're going to need to #if out the second part of the conditional as you're currently doing anyway, or the code won't compile. What I was suggesting was that you use NOTREACHED() in place of LOG(FATAL). We try to avoid logging except in specific scenarios where we need to be able to collect and see the log data; it basically means "this shouldn't happen, but it does, and we're debugging why". NOTREACHED, OTOH, means that the codepath is never reached. That's the case here.
estade@chromium.org changed reviewers: + pinkerton@chromium.org
On 2015/09/08 20:48:13, ki.stfu wrote: > On 2015/09/08 19:20:48, Evan Stade wrote: > > does this break the mac views build? > > I can't test it on Mac. Can somebody start a build on buildbot to check whether > it breaks Mac build or not? I don't think there's a mac views trybot or buildbot yet. I would +jackhou but he seems to be OOO for the next good while, so +pinkerton do you know who can help us figure out if this works on macviews?
https://codereview.chromium.org/1313383006/diff/1/chrome/browser/password_man... File chrome/browser/password_manager/chrome_password_manager_client.cc (right): https://codereview.chromium.org/1313383006/diff/1/chrome/browser/password_man... chrome/browser/password_manager/chrome_password_manager_client.cc:518: #endif On 2015/09/08 20:39:54, ki.stfu wrote: > On 2015/09/08 12:52:49, vabr (Chromium) wrote: > > The current changes in this method are not ideal, because > > 1. There is no reason to offer the command-line flags for the bubble if we > don't > > want to see it (bonus points if you exclude its definition on Aura platforms > as > > well). > > 2. The corresponding field trial is expired (sorry, I should have mentioned it > > to you, you have no way to check that), so no need to worry about that. > > > > Also, the negations in the ifdefs are getting hard to read. So I suggest the > > following structure of the code: > > > > #if defined(OS_MACOSX) || defined(OS_ANDROID) > > ... (the current checks of flags and field trials) > > #elif defined(USE_AURA) > > return true; > > #else > > return false; > > #endif > > > > I worry about the last case: > ``` > #else > return false; > #endif > ``` > > FMPOV, we should return "true" there, because "false" is only suitable for Mac > and Android platforms. Fair enough. This file only compiles on Android, Mac, Win, Linux and CrOS. The last 3 always define USE_AURA, and the rest is covered by the #if branch. So dropping the "return false" is OK. https://codereview.chromium.org/1313383006/diff/20001/chrome/browser/password... File chrome/browser/password_manager/chrome_password_manager_client.cc (right): https://codereview.chromium.org/1313383006/diff/20001/chrome/browser/password... chrome/browser/password_manager/chrome_password_manager_client.cc:535: // Return true for non-Mac and non-Android platforms, including Actually all the platforms here other than Mac and Android always run Aura (iOS is non-Aura, but does not use this file), so I suggest this formulation: // All other platforms use Aura, and therefore always show the bubble.
https://codereview.chromium.org/1313383006/diff/20001/chrome/browser/password... File chrome/browser/password_manager/chrome_password_manager_client.cc (right): https://codereview.chromium.org/1313383006/diff/20001/chrome/browser/password... chrome/browser/password_manager/chrome_password_manager_client.cc:214: DCHECK(is_the_hot_new_bubble_ui_enabled) << "New bubble UI should be used" Is it ok? Or it's too complicated and I should revert it and use NOTREACHED() in #else branch? https://codereview.chromium.org/1313383006/diff/20001/chrome/browser/password... chrome/browser/password_manager/chrome_password_manager_client.cc:535: // Return true for non-Mac and non-Android platforms, including On 2015/09/09 09:18:20, vabr (Chromium) wrote: > Actually all the platforms here other than Mac and Android always run Aura (iOS > is non-Aura, but does not use this file), so I suggest this formulation: > // All other platforms use Aura, and therefore always show the bubble. Done.
pinkerton@chromium.org changed reviewers: + tapted@chromium.org
Pretty sure we don't have a bot, but tapted@ can answer (of course, he's OOTO too...)
LGTM https://codereview.chromium.org/1313383006/diff/60001/chrome/common/chrome_sw... File chrome/common/chrome_switches.cc (right): https://codereview.chromium.org/1313383006/diff/60001/chrome/common/chrome_sw... chrome/common/chrome_switches.cc:1172: const char kEnableSavePasswordBubble[] = "enable-save-password-bubble"; Nit: Remove extra spaces
On 2015/09/08 21:03:13, Evan Stade wrote: > On 2015/09/08 20:48:13, ki.stfu wrote: > > On 2015/09/08 19:20:48, Evan Stade wrote: > > > does this break the mac views build? > > > > I can't test it on Mac. Can somebody start a build on buildbot to check > whether > > it breaks Mac build or not? > > I don't think there's a mac views trybot or buildbot yet. I would +jackhou but > he seems to be OOO for the next good while, so +pinkerton do you know who can > help us figure out if this works on macviews? Don't worry about macviews :). There's no special build configuration for the bits of MacViews that are currently of interest, so if the mac bots are happy, so is macviews. We just don't run the `views_unittests` target by default on Mac trybots yet (regressions are rare, so I just check up on it occasionally). There *is* a mac_views_browser GYP flag which is more forward-looking - it worked last I checked, but it doesn't matter too much if it breaks for a while. This CL might* break mac_views_browser (or, at best, compile in some unnecessary code there), so I'll probably need to follow up. * I tried patching it in, but my laptop checkout is too old (and my hotel wifi is probably not up to the task of a pull/sync).
Some more comments. Thanks, Vaclav https://codereview.chromium.org/1313383006/diff/60001/chrome/browser/about_fl... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/1313383006/diff/60001/chrome/browser/about_fl... chrome/browser/about_flags.cc:1581: #if !defined(USE_AURA) Note that Win, Linux and CrOS always have Aura defined, so the current code does not enable the flag at all. What we wanted was to enable the flag only on Mac (independent of whether Aura or Mac native UI support is used), so I suggest #if defined(OS_MACOSX) and changing line 1585 to just kOsMac. https://codereview.chromium.org/1313383006/diff/60001/chrome/browser/password... File chrome/browser/password_manager/chrome_password_manager_client.cc (right): https://codereview.chromium.org/1313383006/diff/60001/chrome/browser/password... chrome/browser/password_manager/chrome_password_manager_client.cc:518: if (command_line->HasSwitch(switches::kDisableSavePasswordBubble)) { nit: Drop the { and }, for on-liner "if" it is not needed, and dropping it is locally consistent with the rest of the file. https://codereview.chromium.org/1313383006/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/infobars/save_password_infobar.cc (left): https://codereview.chromium.org/1313383006/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/infobars/save_password_infobar.cc:8: scoped_ptr<infobars::InfoBar> CreateSavePasswordInfoBar( Isn't this used by Android (and Mac with views)? https://codereview.chromium.org/1313383006/diff/60001/chrome/common/chrome_sw... File chrome/common/chrome_switches.h (right): https://codereview.chromium.org/1313383006/diff/60001/chrome/common/chrome_sw... chrome/common/chrome_switches.h:333: #if !defined(USE_AURA) Please change to #if defined(OS_MACOSX) (see about_flags.cc above). Also in the .cc file here.
https://codereview.chromium.org/1313383006/diff/60001/chrome/browser/about_fl... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/1313383006/diff/60001/chrome/browser/about_fl... chrome/browser/about_flags.cc:1581: #if !defined(USE_AURA) On 2015/09/10 07:21:47, vabr (Chromium) wrote: > Note that Win, Linux and CrOS always have Aura defined, so the current code does > not enable the flag at all. > > What we wanted was to enable the flag only on Mac (independent of whether Aura > or Mac native UI support is used), so I suggest > #if defined(OS_MACOSX) > and > changing line 1585 to just kOsMac. Done. https://codereview.chromium.org/1313383006/diff/60001/chrome/browser/password... File chrome/browser/password_manager/chrome_password_manager_client.cc (right): https://codereview.chromium.org/1313383006/diff/60001/chrome/browser/password... chrome/browser/password_manager/chrome_password_manager_client.cc:518: if (command_line->HasSwitch(switches::kDisableSavePasswordBubble)) { On 2015/09/10 07:21:47, vabr (Chromium) wrote: > nit: Drop the { and }, for on-liner "if" it is not needed, and dropping it is > locally consistent with the rest of the file. Done. https://codereview.chromium.org/1313383006/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/infobars/save_password_infobar.cc (left): https://codereview.chromium.org/1313383006/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/infobars/save_password_infobar.cc:8: scoped_ptr<infobars::InfoBar> CreateSavePasswordInfoBar( On 2015/09/10 07:21:47, vabr (Chromium) wrote: > Isn't this used by Android (and Mac with views)? I think no. Android has its own version of CreateSavePasswordInfoBar, which is implemented here: chrome/browser/ui/android/infobars/save_password_infobar.cc. https://codereview.chromium.org/1313383006/diff/60001/chrome/common/chrome_sw... File chrome/common/chrome_switches.cc (right): https://codereview.chromium.org/1313383006/diff/60001/chrome/common/chrome_sw... chrome/common/chrome_switches.cc:1172: const char kEnableSavePasswordBubble[] = "enable-save-password-bubble"; On 2015/09/09 20:27:01, Peter Kasting wrote: > Nit: Remove extra spaces Done. https://codereview.chromium.org/1313383006/diff/60001/chrome/common/chrome_sw... File chrome/common/chrome_switches.h (right): https://codereview.chromium.org/1313383006/diff/60001/chrome/common/chrome_sw... chrome/common/chrome_switches.h:333: #if !defined(USE_AURA) On 2015/09/10 07:21:47, vabr (Chromium) wrote: > Please change to > #if defined(OS_MACOSX) > (see about_flags.cc above). > Also in the .cc file here. Done.
Thanks! Patch set 5 LGTM. @tapted -- if you hit problems related to this in Mac Views, feel free to ping me for help. Cheers, Vaclav https://codereview.chromium.org/1313383006/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/infobars/save_password_infobar.cc (left): https://codereview.chromium.org/1313383006/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/infobars/save_password_infobar.cc:8: scoped_ptr<infobars::InfoBar> CreateSavePasswordInfoBar( On 2015/09/10 18:21:00, ki.stfu wrote: > On 2015/09/10 07:21:47, vabr (Chromium) wrote: > > Isn't this used by Android (and Mac with views)? > > I think no. Android has its own version of CreateSavePasswordInfoBar, which is > implemented here: chrome/browser/ui/android/infobars/save_password_infobar.cc. Fair enough. The Android trybot also compiled patch set 4 without problems.
The CQ bit was checked by ki.stfu@gmail.com
The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/1313383006/#ps80001 (title: "Make a bit refactoring")
The CQ bit was unchecked by ki.stfu@gmail.com
The CQ bit was checked by ki.stfu@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1313383006/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1313383006/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
Since you need some .gn changes, you may consider a simple change below that will keep mac_views_browser happy as well. But I'm happy to explore that in a follow-up too. https://codereview.chromium.org/1313383006/diff/80001/chrome/chrome_browser_u... File chrome/chrome_browser_ui.gypi (left): https://codereview.chromium.org/1313383006/diff/80001/chrome/chrome_browser_u... chrome/chrome_browser_ui.gypi:2218: 'browser/ui/views/infobars/save_password_infobar.cc', To keep mac_views_browser happy this just needs to move to chrome_browser_ui_views_mac_experimental_sources rather than being deleted (see https://codereview.chromium.org/1339963002/#ps20001 ) I think that's the nicest/least intrusive way to do this change without breaking the mac_views_browser compile. Alternatives would involve adding additional complexity to ChromePasswordManagerClient and not really achieve much. https://codereview.chromium.org/1313383006/diff/80001/chrome/chrome_tests_uni... File chrome/chrome_tests_unit.gypi (right): https://codereview.chromium.org/1313383006/diff/80001/chrome/chrome_tests_uni... chrome/chrome_tests_unit.gypi:2535: 'sources': [ '<@(chrome_unit_tests_mac_android_sources)' ], Structural changes like this need a corresponding BUILD.gn change as well. See https://codereview.chromium.org/1339963002/#ps40001
On 2015/09/14 07:07:05, tapted wrote: > Since you need some .gn changes, you may consider a simple change below that > will keep mac_views_browser happy as well. But I'm happy to explore that in a > follow-up too. > > https://codereview.chromium.org/1313383006/diff/80001/chrome/chrome_browser_u... > File chrome/chrome_browser_ui.gypi (left): > > https://codereview.chromium.org/1313383006/diff/80001/chrome/chrome_browser_u... > chrome/chrome_browser_ui.gypi:2218: > 'browser/ui/views/infobars/save_password_infobar.cc', > To keep mac_views_browser happy this just needs to move to > chrome_browser_ui_views_mac_experimental_sources rather than being deleted (see > https://codereview.chromium.org/1339963002/#ps20001 ) > > I think that's the nicest/least intrusive way to do this change without breaking > the mac_views_browser compile. > > Alternatives would involve adding additional complexity to > ChromePasswordManagerClient and not really achieve much. > > https://codereview.chromium.org/1313383006/diff/80001/chrome/chrome_tests_uni... > File chrome/chrome_tests_unit.gypi (right): > > https://codereview.chromium.org/1313383006/diff/80001/chrome/chrome_tests_uni... > chrome/chrome_tests_unit.gypi:2535: 'sources': [ > '<@(chrome_unit_tests_mac_android_sources)' ], > Structural changes like this need a corresponding BUILD.gn change as well. See > https://codereview.chromium.org/1339963002/#ps40001 Thank you.
https://codereview.chromium.org/1313383006/diff/80001/chrome/chrome_browser_u... File chrome/chrome_browser_ui.gypi (left): https://codereview.chromium.org/1313383006/diff/80001/chrome/chrome_browser_u... chrome/chrome_browser_ui.gypi:2218: 'browser/ui/views/infobars/save_password_infobar.cc', On 2015/09/14 07:07:05, tapted wrote: > To keep mac_views_browser happy this just needs to move to > chrome_browser_ui_views_mac_experimental_sources rather than being deleted (see > https://codereview.chromium.org/1339963002/#ps20001 ) > > I think that's the nicest/least intrusive way to do this change without breaking > the mac_views_browser compile. > > Alternatives would involve adding additional complexity to > ChromePasswordManagerClient and not really achieve much. Done. https://codereview.chromium.org/1313383006/diff/80001/chrome/chrome_tests_uni... File chrome/chrome_tests_unit.gypi (right): https://codereview.chromium.org/1313383006/diff/80001/chrome/chrome_tests_uni... chrome/chrome_tests_unit.gypi:2535: 'sources': [ '<@(chrome_unit_tests_mac_android_sources)' ], On 2015/09/14 07:07:05, tapted wrote: > Structural changes like this need a corresponding BUILD.gn change as well. See > https://codereview.chromium.org/1339963002/#ps40001 Done.
The CQ bit was checked by ki.stfu@gmail.com
The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org, pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/1313383006/#ps140001 (title: "Fix GN build")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1313383006/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1313383006/140001
The CQ bit was unchecked by ki.stfu@gmail.com
The CQ bit was checked by ki.stfu@gmail.com
The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org, pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/1313383006/#ps160001 (title: "Rebase against ToT")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1313383006/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1313383006/160001
ki.stfu@gmail.com changed reviewers: + sky@chromium.org
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...)
https://codereview.chromium.org/1313383006/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/infobars/save_password_infobar.cc (left): https://codereview.chromium.org/1313383006/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/infobars/save_password_infobar.cc:7: Forgot to add tapted@'s comment here: // TODO(tapted): Delete this file when Mac uses the password bubble by default.
lgtm fwiw, but I don't give you any OWNER tokens. I see you've added sky@
already. Usually it's good to send an email when adding reviewers, otherwise
it's unlikely they will see it.
E.g. "+sky for OWNERS for chrome/browser/BUILD.gn chrome/test/BUILD.gn"
(
The remaining problem is
** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
chrome/browser/BUILD.gn
chrome/test/BUILD.gn
)
https://codereview.chromium.org/1313383006/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/infobars/save_password_infobar.cc (left):
https://codereview.chromium.org/1313383006/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/infobars/save_password_infobar.cc:7:
On 2015/09/15 11:06:53, ki.stfu wrote:
> Forgot to add tapted@'s comment here:
>
> // TODO(tapted): Delete this file when Mac uses the password bubble by
default.
(note I don't see a change to save_password_infobar.cc in the latest patchset,
but I'm indifferent about whether this gets added -- there's already a TODO in
the gypi -- i.e. to remove that entire gyp variable at some point, and the file
should get reaped at the same time)
https://codereview.chromium.org/1313383006/diff/160001/chrome/chrome_browser.... File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/1313383006/diff/160001/chrome/chrome_browser.... chrome/chrome_browser.gypi:2076: 'browser/password_manager/save_password_infobar_delegate.cc', optional nit: It doesn't seem like this configuration (mac and android only) is going to be very popular so maybe follow the model of browser/net/disk_cache_dir_policy_handler.cc (just a custom condition, no special list).
LGTM
https://codereview.chromium.org/1313383006/diff/160001/chrome/chrome_browser.... File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/1313383006/diff/160001/chrome/chrome_browser.... chrome/chrome_browser.gypi:2076: 'browser/password_manager/save_password_infobar_delegate.cc', On 2015/09/16 00:05:06, Evan Stade (ooo) wrote: > optional nit: It doesn't seem like this configuration (mac and android only) is > going to be very popular so maybe follow the model of > browser/net/disk_cache_dir_policy_handler.cc (just a custom condition, no > special list). It's done not because of popularity of that configuration. As for me, it's a bit clearer than just adding files in conditions.
https://codereview.chromium.org/1313383006/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/infobars/save_password_infobar.cc (left): https://codereview.chromium.org/1313383006/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/infobars/save_password_infobar.cc:7: On 2015/09/15 23:55:32, tapted wrote: > On 2015/09/15 11:06:53, ki.stfu wrote: > > Forgot to add tapted@'s comment here: > > > > // TODO(tapted): Delete this file when Mac uses the password bubble by > default. > > (note I don't see a change to save_password_infobar.cc in the latest patchset, > but I'm indifferent about whether this gets added -- there's already a TODO in > the gypi -- i.e. to remove that entire gyp variable at some point, and the file > should get reaped at the same time) Agreed. But TODO in .c file is seen to everyone who interested in this file, in contrast to the TODO in gypi.
The CQ bit was checked by ki.stfu@gmail.com
The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org, pkasting@chromium.org, sky@chromium.org, vabr@chromium.org Link to the patchset: https://codereview.chromium.org/1313383006/#ps180001 (title: "Add TODO in chrome/browser/ui/views/infobars/save_password_infobar.cc")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1313383006/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1313383006/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by ki.stfu@gmail.com
The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org, pkasting@chromium.org, sky@chromium.org, vabr@chromium.org Link to the patchset: https://codereview.chromium.org/1313383006/#ps200001 (title: "Rebase against ToT")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1313383006/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1313383006/200001
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...)
The CQ bit was checked by ki.stfu@gmail.com
The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org, pkasting@chromium.org, sky@chromium.org, vabr@chromium.org Link to the patchset: https://codereview.chromium.org/1313383006/#ps220001 (title: "Rebase against ToT")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1313383006/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1313383006/220001
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/5e669f04cbfb8655af0afe5854def2263018e1be Cr-Commit-Position: refs/heads/master@{#349370} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
