Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
159853003:
Moving the autofill enum from blink side to browser side (Closed)
|
Created:
6 years, 10 months ago by gnana Modified:
6 years, 10 months ago CC:
chromium-reviews, benquan, tfarina, browser-components-watch_chromium.org, Dane Wallinga, dyu1, estade+watch_chromium.org, Ilya Sherman, rouslan+autofillwatch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionMoving the autofill enum from blink side to browser side
Removing the autofill enum from blink side and moving it to
browser side in components/autofill/core/common/autofill_enums.h.
This patch is only adding of enum in browser side. Removing of
enum from blink side is done in another patch.
BUG=302489
TBR=thakis@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=252053
Patch Set 1 : #
Total comments: 28
Patch Set 2 : incorporated review comments #
Total comments: 8
Patch Set 3 : Rebase & incorporated review comments #
Total comments: 4
Patch Set 4 : incorporated review comments #Patch Set 5 : rebase #Messages
Total messages: 46 (0 generated)
Please have a look.
https://codereview.chromium.org/159853003/diff/30001/chrome/browser/ui/autofi... File chrome/browser/ui/autofill/autofill_popup_controller_impl.cc (right): https://codereview.chromium.org/159853003/diff/30001/chrome/browser/ui/autofi... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:234: values.size(), bad indent https://codereview.chromium.org/159853003/diff/30001/chrome/browser/ui/autofi... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:549: return id != MenuItemIDSeparator && unwrap? https://codereview.chromium.org/159853003/diff/30001/chrome/browser/ui/autofi... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:556: identifiers_[0] == un-wrap https://codereview.chromium.org/159853003/diff/30001/chrome/browser/ui/views/... File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): https://codereview.chromium.org/159853003/diff/30001/chrome/browser/ui/views/... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:55: if (controller_->identifiers()[i] == MenuItemIDSeparator) { remove a space after ==
+isherman Ilya should look at this CL too. https://codereview.chromium.org/159853003/diff/30001/components/autofill/core... File components/autofill/core/common/autofill_enums.h (right): https://codereview.chromium.org/159853003/diff/30001/components/autofill/core... components/autofill/core/common/autofill_enums.h:15: MenuItemIDAutocompleteEntry = 0, I would think this most naturally belongs in autofill_popup_controller.h. Also, the indentation here isn't matching Chromium style (you could consider just running "git cl format" over your patch before uploading -- it will take care of a lot of these style issues).
https://codereview.chromium.org/159853003/diff/30001/chrome/browser/ui/autofi... File chrome/browser/ui/autofill/autofill_popup_controller_impl.cc (right): https://codereview.chromium.org/159853003/diff/30001/chrome/browser/ui/autofi... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:191: identifiers_[0] == MenuItemIDDataListEntry) { nit: Looks like this line no longer needs to wrap. https://codereview.chromium.org/159853003/diff/30001/chrome/browser/ui/autofi... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:202: identifiers_[0] == MenuItemIDSeparator) { nit: Looks like this line no longer needs to wrap. https://codereview.chromium.org/159853003/diff/30001/chrome/browser/ui/autofi... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:220: identifiers_[0] != MenuItemIDSeparator) { nit: Looks like this line no longer needs to wrap. https://codereview.chromium.org/159853003/diff/30001/components/autofill/core... File components/autofill/core/browser/autofill_external_delegate.cc (right): https://codereview.chromium.org/159853003/diff/30001/components/autofill/core... components/autofill/core/browser/autofill_external_delegate.cc:286: MenuItemIDWarningMessage && nit: Looks like this line no longer needs to wrap. https://codereview.chromium.org/159853003/diff/30001/components/autofill/core... File components/autofill/core/common/autofill_enums.h (right): https://codereview.chromium.org/159853003/diff/30001/components/autofill/core... components/autofill/core/common/autofill_enums.h:1: // Copyright 2013 The Chromium Authors. All rights reserved. nit: 2014 https://codereview.chromium.org/159853003/diff/30001/components/autofill/core... components/autofill/core/common/autofill_enums.h:10: #include <stddef.h> // For size_t nit: size_t isn't used in this header file, so there's probably no need for this #include. https://codereview.chromium.org/159853003/diff/30001/components/autofill/core... components/autofill/core/common/autofill_enums.h:14: enum { nit: The enum should be named, probably "MenuItemID", or "PopupItemID" if you take my suggestion below. https://codereview.chromium.org/159853003/diff/30001/components/autofill/core... components/autofill/core/common/autofill_enums.h:19: MenuItemIDClearForm = -4, nit: Rather than "Menu", "Popup" or "PopupMenu" would probably be slightly more consistent with the code that uses this enum. https://codereview.chromium.org/159853003/diff/30001/components/autofill/core... components/autofill/core/common/autofill_enums.h:21: MenuItemIDDataListEntry = -6 nit: Chromium enum values should be formatted in ALL_CAPS.
Thanks, by the way, for picking up this bug -- much appreciated :)
Thanks all for the review comments. Please check now. https://codereview.chromium.org/159853003/diff/30001/chrome/browser/ui/autofi... File chrome/browser/ui/autofill/autofill_popup_controller_impl.cc (right): https://codereview.chromium.org/159853003/diff/30001/chrome/browser/ui/autofi... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:191: identifiers_[0] == MenuItemIDDataListEntry) { On 2014/02/11 22:21:33, Ilya Sherman wrote: > nit: Looks like this line no longer needs to wrap. Done. https://codereview.chromium.org/159853003/diff/30001/chrome/browser/ui/autofi... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:202: identifiers_[0] == MenuItemIDSeparator) { On 2014/02/11 22:21:33, Ilya Sherman wrote: > nit: Looks like this line no longer needs to wrap. Done. https://codereview.chromium.org/159853003/diff/30001/chrome/browser/ui/autofi... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:220: identifiers_[0] != MenuItemIDSeparator) { On 2014/02/11 22:21:33, Ilya Sherman wrote: > nit: Looks like this line no longer needs to wrap. Done. https://codereview.chromium.org/159853003/diff/30001/chrome/browser/ui/autofi... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:234: values.size(), On 2014/02/11 18:56:39, Evan Stade wrote: > bad indent Done. https://codereview.chromium.org/159853003/diff/30001/chrome/browser/ui/autofi... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:549: return id != MenuItemIDSeparator && On 2014/02/11 18:56:39, Evan Stade wrote: > unwrap? Done. https://codereview.chromium.org/159853003/diff/30001/chrome/browser/ui/autofi... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:556: identifiers_[0] == On 2014/02/11 18:56:39, Evan Stade wrote: > un-wrap Done. https://codereview.chromium.org/159853003/diff/30001/chrome/browser/ui/views/... File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): https://codereview.chromium.org/159853003/diff/30001/chrome/browser/ui/views/... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:55: if (controller_->identifiers()[i] == MenuItemIDSeparator) { On 2014/02/11 18:56:39, Evan Stade wrote: > remove a space after == Done. https://codereview.chromium.org/159853003/diff/30001/components/autofill/core... File components/autofill/core/browser/autofill_external_delegate.cc (right): https://codereview.chromium.org/159853003/diff/30001/components/autofill/core... components/autofill/core/browser/autofill_external_delegate.cc:286: MenuItemIDWarningMessage && On 2014/02/11 22:21:33, Ilya Sherman wrote: > nit: Looks like this line no longer needs to wrap. Done. https://codereview.chromium.org/159853003/diff/30001/components/autofill/core... File components/autofill/core/common/autofill_enums.h (right): https://codereview.chromium.org/159853003/diff/30001/components/autofill/core... components/autofill/core/common/autofill_enums.h:1: // Copyright 2013 The Chromium Authors. All rights reserved. On 2014/02/11 22:21:33, Ilya Sherman wrote: > nit: 2014 Done. https://codereview.chromium.org/159853003/diff/30001/components/autofill/core... components/autofill/core/common/autofill_enums.h:10: #include <stddef.h> // For size_t On 2014/02/11 22:21:33, Ilya Sherman wrote: > nit: size_t isn't used in this header file, so there's probably no need for this > #include. Done. https://codereview.chromium.org/159853003/diff/30001/components/autofill/core... components/autofill/core/common/autofill_enums.h:14: enum { On 2014/02/11 22:21:33, Ilya Sherman wrote: > nit: The enum should be named, probably "MenuItemID", or "PopupItemID" if you > take my suggestion below. Done. https://codereview.chromium.org/159853003/diff/30001/components/autofill/core... components/autofill/core/common/autofill_enums.h:15: MenuItemIDAutocompleteEntry = 0, On 2014/02/11 20:30:33, blundell wrote: > I would think this most naturally belongs in autofill_popup_controller.h. Also, > the indentation here isn't matching Chromium style (you could consider just > running "git cl format" over your patch before uploading -- it will take care of > a lot of these style issues). Applied the git cl format, to resolve indentation and other style issues. I tried moving this to autofill_popup_controller.h. But including autofill_popup_controller.h in autofill_external_deleagate.cc and autofill_manager.cc causes presubmit errors. https://codereview.chromium.org/159853003/diff/30001/components/autofill/core... components/autofill/core/common/autofill_enums.h:19: MenuItemIDClearForm = -4, On 2014/02/11 22:21:33, Ilya Sherman wrote: > nit: Rather than "Menu", "Popup" or "PopupMenu" would probably be slightly more > consistent with the code that uses this enum. Done. https://codereview.chromium.org/159853003/diff/30001/components/autofill/core... components/autofill/core/common/autofill_enums.h:21: MenuItemIDDataListEntry = -6 On 2014/02/11 22:21:33, Ilya Sherman wrote: > nit: Chromium enum values should be formatted in ALL_CAPS. Done.
Thanks for this work! Ilya/Evan, what do you think about moving this enum to autofill_popup_delegate.h? I feel like in Chromium we generally don't have enums in standalone header files, but I defer to your opinion. https://codereview.chromium.org/159853003/diff/140002/chrome/browser/ui/autof... File chrome/browser/ui/autofill/autofill_popup_controller_impl.cc (right): https://codereview.chromium.org/159853003/diff/140002/chrome/browser/ui/autof... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:201: if (!identifiers_.empty() && identifiers_[0] == POPUP_ITEM_ID_SEPERATOR) { nit: SEPARATOR https://codereview.chromium.org/159853003/diff/140002/components/autofill/cor... File components/autofill/core/browser/autofill_external_delegate.cc (left): https://codereview.chromium.org/159853003/diff/140002/components/autofill/cor... components/autofill/core/browser/autofill_external_delegate.cc:12: #include "third_party/WebKit/public/web/WebAutofillClient.h" You can get rid of this entry in the DEPS file in this directory now too.
On 2014/02/13 16:10:58, blundell wrote: > Thanks for this work! Ilya/Evan, what do you think about moving this enum to > autofill_popup_delegate.h? I feel like in Chromium we generally don't have enums > in standalone header files, but I defer to your opinion. That's pretty much exactly what components/autofill/core/browser/field_types.h is, so I think it's perfectly fine to have a standalone file for type definitions (not just enums but typedefs or some structs as well). > > https://codereview.chromium.org/159853003/diff/140002/chrome/browser/ui/autof... > File chrome/browser/ui/autofill/autofill_popup_controller_impl.cc (right): > > https://codereview.chromium.org/159853003/diff/140002/chrome/browser/ui/autof... > chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:201: if > (!identifiers_.empty() && identifiers_[0] == POPUP_ITEM_ID_SEPERATOR) { > nit: SEPARATOR > > https://codereview.chromium.org/159853003/diff/140002/components/autofill/cor... > File components/autofill/core/browser/autofill_external_delegate.cc (left): > > https://codereview.chromium.org/159853003/diff/140002/components/autofill/cor... > components/autofill/core/browser/autofill_external_delegate.cc:12: #include > "third_party/WebKit/public/web/WebAutofillClient.h" > You can get rid of this entry in the DEPS file in this directory now too.
Nearly there, thanks! https://codereview.chromium.org/159853003/diff/140002/components/autofill/cor... File components/autofill/core/common/autofill_enums.h (right): https://codereview.chromium.org/159853003/diff/140002/components/autofill/cor... components/autofill/core/common/autofill_enums.h:12: enum PopUpItemId { nit: "PopUp" -> "Popup" https://codereview.chromium.org/159853003/diff/140002/components/autofill/cor... components/autofill/core/common/autofill_enums.h:24: #endif // COMPONENTS_AUTOFILL_CORE_COMMON_AUTOFILL_ENUMS_H_ IMO, Colin's suggestion of moving this enum to autofill_popup_delegate.h makes sense. If we decide against that, though, then I'd recommend naming this file something more specific than "autofill_enums.h" -- perhaps "popup_item_ids.h" -- and moving it to //components/autofill/core/browser rather than //components/autofill/core/common, assuming that there are no //components/autofill/content/renderer dependencies on this enum.
Thanks, Once again. Please check now. https://codereview.chromium.org/159853003/diff/140002/chrome/browser/ui/autof... File chrome/browser/ui/autofill/autofill_popup_controller_impl.cc (right): https://codereview.chromium.org/159853003/diff/140002/chrome/browser/ui/autof... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:201: if (!identifiers_.empty() && identifiers_[0] == POPUP_ITEM_ID_SEPERATOR) { On 2014/02/13 16:10:58, blundell wrote: > nit: SEPARATOR Applied everywhere. Done. https://codereview.chromium.org/159853003/diff/140002/components/autofill/cor... File components/autofill/core/browser/autofill_external_delegate.cc (left): https://codereview.chromium.org/159853003/diff/140002/components/autofill/cor... components/autofill/core/browser/autofill_external_delegate.cc:12: #include "third_party/WebKit/public/web/WebAutofillClient.h" On 2014/02/13 16:10:58, blundell wrote: > You can get rid of this entry in the DEPS file in this directory now too. Done. https://codereview.chromium.org/159853003/diff/140002/components/autofill/cor... File components/autofill/core/common/autofill_enums.h (right): https://codereview.chromium.org/159853003/diff/140002/components/autofill/cor... components/autofill/core/common/autofill_enums.h:12: enum PopUpItemId { On 2014/02/13 22:30:03, Ilya Sherman wrote: > nit: "PopUp" -> "Popup" Done. https://codereview.chromium.org/159853003/diff/140002/components/autofill/cor... components/autofill/core/common/autofill_enums.h:24: #endif // COMPONENTS_AUTOFILL_CORE_COMMON_AUTOFILL_ENUMS_H_ On 2014/02/13 22:30:03, Ilya Sherman wrote: > IMO, Colin's suggestion of moving this enum to autofill_popup_delegate.h makes > sense. > > If we decide against that, though, then I'd recommend naming this file something > more specific than "autofill_enums.h" -- perhaps "popup_item_ids.h" -- and > moving it to //components/autofill/core/browser rather than > //components/autofill/core/common, assuming that there are no > //components/autofill/content/renderer dependencies on this enum. Done.
LGTM with nits. https://codereview.chromium.org/159853003/diff/290001/components/autofill/cor... File components/autofill/core/browser/popup_item_ids.h (right): https://codereview.chromium.org/159853003/diff/290001/components/autofill/cor... components/autofill/core/browser/popup_item_ids.h:5: // Contains enum specific to the Autofill component. This comment should change to be about what this enum is. https://codereview.chromium.org/159853003/diff/290001/components/autofill/cor... components/autofill/core/browser/popup_item_ids.h:7: #ifndef COMPONENTS_AUTOFILL_CORE_COMMON_AUTOFILL_ENUMS_H_ This should change.
nits applied https://codereview.chromium.org/159853003/diff/290001/components/autofill/cor... File components/autofill/core/browser/popup_item_ids.h (right): https://codereview.chromium.org/159853003/diff/290001/components/autofill/cor... components/autofill/core/browser/popup_item_ids.h:5: // Contains enum specific to the Autofill component. On 2014/02/14 16:01:11, blundell wrote: > This comment should change to be about what this enum is. Done. https://codereview.chromium.org/159853003/diff/290001/components/autofill/cor... components/autofill/core/browser/popup_item_ids.h:7: #ifndef COMPONENTS_AUTOFILL_CORE_COMMON_AUTOFILL_ENUMS_H_ On 2014/02/14 16:01:11, blundell wrote: > This should change. Done.
LGTM, thanks!
The CQ bit was checked by gnanasekar.s@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gnanasekar.s@samsung.com/159853003/350001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for
components/autofill/core/browser/autofill_manager_unittest.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file components/autofill/core/browser/autofill_manager_unittest.cc
Hunk #2 FAILED at 48.
Hunk #3 succeeded at 917 (offset -10 lines).
1 out of 3 hunks FAILED -- saving rejects to file
components/autofill/core/browser/autofill_manager_unittest.cc.rej
Patch: components/autofill/core/browser/autofill_manager_unittest.cc
Index: components/autofill/core/browser/autofill_manager_unittest.cc
diff --git a/components/autofill/core/browser/autofill_manager_unittest.cc
b/components/autofill/core/browser/autofill_manager_unittest.cc
index
3482e8604a0987f2ca90487c3243c28a8052d1b4..fece5a7c69853bf064b21459278a9f261d2b58a6
100644
--- a/components/autofill/core/browser/autofill_manager_unittest.cc
+++ b/components/autofill/core/browser/autofill_manager_unittest.cc
@@ -33,6 +33,7 @@
#include "components/autofill/core/browser/autofill_test_utils.h"
#include "components/autofill/core/browser/credit_card.h"
#include "components/autofill/core/browser/personal_data_manager.h"
+#include "components/autofill/core/browser/popup_item_ids.h"
#include "components/autofill/core/browser/test_autofill_driver.h"
#include "components/autofill/core/browser/test_autofill_external_delegate.h"
#include "components/autofill/core/browser/test_autofill_manager_delegate.h"
@@ -47,7 +48,6 @@
#include "grit/component_strings.h"
#include "testing/gmock/include/gmock/gmock.h"
#include "testing/gtest/include/gtest/gtest.h"
-#include "third_party/WebKit/public/web/WebAutofillClient.h"
#include "third_party/WebKit/public/web/WebFormElement.h"
#include "ui/base/l10n/l10n_util.h"
#include "ui/gfx/rect.h"
@@ -926,8 +926,7 @@ TEST_F(AutofillManagerTest, GetProfileSuggestionsMethodGet)
{
};
base::string16 expected_labels[] = {base::string16()};
base::string16 expected_icons[] = {base::string16()};
- int expected_unique_ids[] =
- {blink::WebAutofillClient::MenuItemIDWarningMessage};
+ int expected_unique_ids[] = {POPUP_ITEM_ID_WARNING_MESSAGE};
external_delegate_->CheckSuggestions(
kDefaultPageID, arraysize(expected_values), expected_values,
expected_labels, expected_icons, expected_unique_ids);
The CQ bit was checked by gnanasekar.s@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gnanasekar.s@samsung.com/159853003/440001
The CQ bit was unchecked by gnanasekar.s@samsung.com
On 2014/02/15 07:10:18, gnana wrote:
> The CQ bit was unchecked by mailto:gnanasekar.s@samsung.com
Chromium presubmit failed with below error.
Missing LGTM from an OWNER for these files:
chrome/browser/DEPS
chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc
chrome/browser/ui/views/autofill/autofill_popup_view_views.cc
Please rubber stamp Whole CL
rslgtm
The CQ bit was checked by isherman@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gnanasekar.s@samsung.com/159853003/460003
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
TBR Nico for a trivial DEPS change.
The CQ bit was checked by isherman@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gnanasekar.s@samsung.com/159853003/460003
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel, mac_chromium_rel
The CQ bit was checked by isherman@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gnanasekar.s@samsung.com/159853003/460003
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel, mac_chromium_rel
trivial lgtm
The CQ bit was checked by isherman@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gnanasekar.s@samsung.com/159853003/460003
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel, mac_chromium_rel
The CQ bit was checked by gnanasekar.s@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gnanasekar.s@samsung.com/159853003/460003
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gnanasekar.s@samsung.com/159853003/460003
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gnanasekar.s@samsung.com/159853003/460003
Message was sent while issue was closed.
Change committed as 252053 |
