Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(133)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 21124012: [WIP] Split AutofillDialogControllerImpl + integrate rAc on Android. (Closed)

Created:
7 years, 4 months ago by aruslan
Modified:
7 years, 4 months ago
Reviewers:
Evan Stade
CC:
chromium-reviews, Raman Kakilate, benquan, ahutter, browser-components-watch_chromium.org, Dane Wallinga, dyu1, Albert Bodenhamer, estade+watch_chromium.org, Ilya Sherman, rouslan+autofillwatch_chromium.org
Visibility:
Public.

Description

[WIP] rAc dialog split and integration on Android. BUG=259089 TEST=TODO

Patch Set 1 #

Total comments: 30
Unified diffs Side-by-side diffs Delta from patch set Stats (+1534 lines, -183 lines) Patch
M build/common.gypi View 1 chunk +1 line, -1 line 0 comments Download
A chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogResult.java View 1 chunk +232 lines, -0 lines 0 comments Download
A chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogTabManagerDelegateAndroid.java View 1 chunk +138 lines, -0 lines 0 comments Download
M chrome/browser/android/chrome_jni_registrar.cc View 2 chunks +7 lines, -3 lines 0 comments Download
M chrome/browser/prefs/browser_prefs.cc View 2 chunks +2 lines, -2 lines 0 comments Download
A chrome/browser/ui/android/autofill/autofill_dialog_result.h View 1 chunk +40 lines, -0 lines 2 comments Download
A chrome/browser/ui/android/autofill/autofill_dialog_result.cc View 1 chunk +124 lines, -0 lines 0 comments Download
A chrome/browser/ui/android/autofill/autofill_dialog_tab_manager_delegate_android.h View 1 chunk +134 lines, -0 lines 0 comments Download
A chrome/browser/ui/android/autofill/autofill_dialog_tab_manager_delegate_android.cc View 1 chunk +457 lines, -0 lines 0 comments Download
M chrome/browser/ui/autofill/autofill_dialog_controller_impl.h View 4 chunks +12 lines, -9 lines 0 comments Download
M chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc View 17 chunks +45 lines, -137 lines 6 comments Download
A chrome/browser/ui/autofill/autofill_dialog_tab_manager_delegate.h View 1 chunk +70 lines, -0 lines 12 comments Download
chrome/browser/ui/autofill/autofill_dialog_tab_manager_delegate.cc View 1 chunk +27 lines, -0 lines 4 comments Download
A chrome/browser/ui/autofill/autofill_dialog_utils.h View 1 chunk +40 lines, -0 lines 2 comments Download
A chrome/browser/ui/autofill/autofill_dialog_utils.cc View 1 chunk +149 lines, -0 lines 2 comments Download
M chrome/browser/ui/autofill/tab_autofill_manager_delegate.h View 3 chunks +4 lines, -4 lines 0 comments Download
M chrome/browser/ui/autofill/tab_autofill_manager_delegate.cc View 9 chunks +31 lines, -24 lines 2 comments Download
M chrome/chrome_browser.gypi View 1 chunk +2 lines, -1 line 0 comments Download
M chrome/chrome_browser_ui.gypi View 3 chunks +14 lines, -2 lines 0 comments Download
M chrome/common/pref_names.h View 1 chunk +1 line, -0 lines 0 comments Download
M chrome/common/pref_names.cc View 1 chunk +4 lines, -0 lines 0 comments Download

Messages

Total messages: 6 (0 generated)
aruslan
Evan -- please take a look at the controller/tab-manager-delegate split. Name suggestions for the AutofillDialogTabManagerDelegate ...
7 years, 4 months ago (2013-07-31 02:35:30 UTC) #1
aruslan
https://codereview.chromium.org/21124012/diff/1/chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right): https://codereview.chromium.org/21124012/diff/1/chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc#newcode2300 chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:2300: #if defined(TOOLKIT_VIEWS) This should be #if defined(ENABLE_AUTOFILL_DIALOG). https://codereview.chromium.org/21124012/diff/1/chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc#newcode2519 chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:2519: ...
7 years, 4 months ago (2013-07-31 02:43:18 UTC) #2
aruslan
7 years, 4 months ago (2013-08-01 17:34:02 UTC) #3
aruslan
https://chromiumcodereview.appspot.com/21124012/diff/1/chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right): https://chromiumcodereview.appspot.com/21124012/diff/1/chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc#newcode2300 chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:2300: #if defined(TOOLKIT_VIEWS) On 2013/07/31 02:43:19, aruslan wrote: > This ...
7 years, 4 months ago (2013-08-01 19:30:29 UTC) #4
Evan Stade
https://chromiumcodereview.appspot.com/21124012/diff/1/chrome/browser/ui/android/autofill/autofill_dialog_result.h File chrome/browser/ui/android/autofill/autofill_dialog_result.h (right): https://chromiumcodereview.appspot.com/21124012/diff/1/chrome/browser/ui/android/autofill/autofill_dialog_result.h#newcode22 chrome/browser/ui/android/autofill/autofill_dialog_result.h:22: // TODO(aruslan): name and description; should be moved into ...
7 years, 4 months ago (2013-08-01 19:44:51 UTC) #5
aruslan
7 years, 4 months ago (2013-08-07 23:17:02 UTC) #6 
Thanks, Evan!
The following CLs came out of this CL:
https://chromiumcodereview.appspot.com/21205007/ [rAc Android] Remove the old
dialog implementation.
https://chromiumcodereview.appspot.com/21692002/ Rename AutofillDialogController
to AutofillDialogViewDelegate.
https://chromiumcodereview.appspot.com/21928004/ Add a FullWallet constructor
for data obtained from SDK.
https://chromiumcodereview.appspot.com/22623002/ Extract
AutofillDialogController interface and common utilities.
https://chromiumcodereview.appspot.com/22566004/ [WIP] [rAc Android]
Integration.

PTAL at the Extract AutofillDialogController
https://chromiumcodereview.appspot.com/22623002/, thanks!

https://chromiumcodereview.appspot.com/21124012/diff/1/chrome/browser/ui/andr...
File chrome/browser/ui/android/autofill/autofill_dialog_result.h (right):

https://chromiumcodereview.appspot.com/21124012/diff/1/chrome/browser/ui/andr...
chrome/browser/ui/android/autofill/autofill_dialog_result.h:22: //
TODO(aruslan): name and description; should be moved into a separate .h.
On 2013/08/01 19:44:52, Evan Stade wrote:
> ^

Done.

https://chromiumcodereview.appspot.com/21124012/diff/1/chrome/browser/ui/auto...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://chromiumcodereview.appspot.com/21124012/diff/1/chrome/browser/ui/auto...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:2300: #if
defined(TOOLKIT_VIEWS)
On 2013/08/01 19:30:29, aruslan wrote:
> On 2013/07/31 02:43:19, aruslan wrote:
> > This should be #if defined(ENABLE_AUTOFILL_DIALOG).
> 
> Actually, #if should be removed entirely.

Done.

https://chromiumcodereview.appspot.com/21124012/diff/1/chrome/browser/ui/auto...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:2300: #if
defined(TOOLKIT_VIEWS)
On 2013/07/31 02:43:19, aruslan wrote:
> This should be #if defined(ENABLE_AUTOFILL_DIALOG).

Done: removed entirely.

https://chromiumcodereview.appspot.com/21124012/diff/1/chrome/browser/ui/auto...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:2519:
requested_shipping_fields_) ||
On 2013/07/31 02:43:19, aruslan wrote:
> indent.

Done.

https://chromiumcodereview.appspot.com/21124012/diff/1/chrome/browser/ui/auto...
File chrome/browser/ui/autofill/autofill_dialog_tab_manager_delegate.cc (right):

https://chromiumcodereview.appspot.com/21124012/diff/1/chrome/browser/ui/auto...
chrome/browser/ui/autofill/autofill_dialog_tab_manager_delegate.cc:9:
AutofillDialogTabManagerDelegate::~AutofillDialogTabManagerDelegate() {
On 2013/08/01 19:44:52, Evan Stade wrote:
> {} on same line

Done.

https://chromiumcodereview.appspot.com/21124012/diff/1/chrome/browser/ui/auto...
chrome/browser/ui/autofill/autofill_dialog_tab_manager_delegate.cc:12: #if
!defined(TOOLKIT_VIEWS) && !defined(OS_ANDROID)
On 2013/07/31 02:43:19, aruslan wrote:
> This should be #if !defined(ENABLE_AUTOFILL_DIALOG).

Done.

https://chromiumcodereview.appspot.com/21124012/diff/1/chrome/browser/ui/auto...
File chrome/browser/ui/autofill/autofill_dialog_tab_manager_delegate.h (right):

https://chromiumcodereview.appspot.com/21124012/diff/1/chrome/browser/ui/auto...
chrome/browser/ui/autofill/autofill_dialog_tab_manager_delegate.h:29: //
TODO(aruslan): name and description; should be moved into a separate .h.
On 2013/07/31 02:43:19, aruslan wrote:
> Description.

Done.

https://chromiumcodereview.appspot.com/21124012/diff/1/chrome/browser/ui/auto...
chrome/browser/ui/autofill/autofill_dialog_tab_manager_delegate.h:30: class
AutofillDialogTabManagerDelegate {
On 2013/08/01 19:44:52, Evan Stade wrote:
> this name is super confusing. Should be AutofillDialogController. The thing
that
> is currently called AutofillDialogController can be renamed to
> AutofillDialogViewDelegate.

Done.

https://chromiumcodereview.appspot.com/21124012/diff/1/chrome/browser/ui/auto...
chrome/browser/ui/autofill/autofill_dialog_tab_manager_delegate.h:34: static
base::WeakPtr<AutofillDialogTabManagerDelegate> Create(
On 2013/07/31 02:43:19, aruslan wrote:
> Description

Done.

https://chromiumcodereview.appspot.com/21124012/diff/1/chrome/browser/ui/auto...
chrome/browser/ui/autofill/autofill_dialog_tab_manager_delegate.h:42: static
void RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry);
On 2013/07/31 02:43:19, aruslan wrote:
> Description.

Done.

https://chromiumcodereview.appspot.com/21124012/diff/1/chrome/browser/ui/auto...
chrome/browser/ui/autofill/autofill_dialog_tab_manager_delegate.h:44: virtual
void Show() = 0;
On 2013/07/31 02:43:19, aruslan wrote:
> Descriptions.

Done.

https://chromiumcodereview.appspot.com/21124012/diff/1/chrome/browser/ui/auto...
chrome/browser/ui/autofill/autofill_dialog_tab_manager_delegate.h:65: virtual
DialogType GetDialogType() const = 0;
On 2013/07/31 02:43:19, aruslan wrote:
> Description.

Done.

https://chromiumcodereview.appspot.com/21124012/diff/1/chrome/browser/ui/auto...
File chrome/browser/ui/autofill/autofill_dialog_utils.cc (right):

https://chromiumcodereview.appspot.com/21124012/diff/1/chrome/browser/ui/auto...
chrome/browser/ui/autofill/autofill_dialog_utils.cc:19: const DetailInput
kEmailInputs[] = {
On 2013/08/01 19:44:52, Evan Stade wrote:
> I think it's better not to statically initialize all this.

Done.

https://chromiumcodereview.appspot.com/21124012/diff/1/chrome/browser/ui/auto...
File chrome/browser/ui/autofill/autofill_dialog_utils.h (right):

https://chromiumcodereview.appspot.com/21124012/diff/1/chrome/browser/ui/auto...
chrome/browser/ui/autofill/autofill_dialog_utils.h:20: namespace utils {
On 2013/08/01 19:44:52, Evan Stade wrote:
> I would call this common, not utils. These are mostly functions for operating
on
> the types in autofill_dialog_types.h so you should also move the static
> functions from autofill_dialog_types to here.

Done: renamed to common.

https://chromiumcodereview.appspot.com/21124012/diff/1/chrome/browser/ui/auto...
File chrome/browser/ui/autofill/tab_autofill_manager_delegate.cc (right):

https://chromiumcodereview.appspot.com/21124012/diff/1/chrome/browser/ui/auto...
chrome/browser/ui/autofill/tab_autofill_manager_delegate.cc:149: #if
defined(ENABLE_AUTOFILL_DIALOG)
On 2013/08/01 19:44:52, Evan Stade wrote:
> I don't think you need this ifdef. Instead do
> 
> if (dialog_delegate_)
>   dialog_delegate_->Show();
> else
>   NOTIMPL()
>   callback()

Done.

Powered by Google App Engine
This is Rietveld 408576698