Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
710453002:
[Autofill] Componentize AutofillCCInfoBarDelegate. (Closed)
|
Created:
6 years, 1 month ago by Pritam Nikam Modified:
5 years, 11 months ago Reviewers:
droger, Peter Kasting, sky, oshima, Evan Stade, benm (inactive), blundell, Ilya Sherman, dconnelly, sdefresne CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Autofill] Componentize AutofillCCInfoBarDelegate.
The main tasks involved:
- Changing InfoBarService references to InfoBarManager references.
- Abstracting the call to WebContents::OpenURL() through the AutofillClient, which has been passed through to the AutofillCCInfoBarDelegate.
- Moving strings/resources that the AutofillCCInfoBarDelegate uses into the components.
Pending activities to follow-up:
- Componentize AutofillCCInfoBarDelegate unit-tests [Currently, having many |/chrome| dependencies].
BUG=382924
Committed: https://crrev.com/2cd65ab9548cd1c4bca4e0da1f8a59a531d82566
Cr-Commit-Position: refs/heads/master@{#311238}
Patch Set 1 : Changing InfoBarService references to InfoBarManager references. #Patch Set 2 : Componentize AutofillCCInfoBarDelegate. #Patch Set 3 : Modified AutofillCCInfoBarDelegate unit-tests. #
Total comments: 10
Patch Set 4 : Abstracting the call to WebContents::OpenURL() throuh the AutofillDriver. #
Total comments: 19
Patch Set 5 : Incorporated review comments. #Patch Set 6 : Moved |autofill_cc_infobar_delegate.h/cc| under |components/autofill/core/browser|. #Patch Set 7 : Return nullptr for CreateInfoBar() in test_autofill_client.cc. #
Total comments: 23
Patch Set 8 : Incorporated Ilya's review inputs. #Patch Set 9 : Rebased. #
Total comments: 9
Patch Set 10 : Incorporates Ilya's review inputs. #Patch Set 11 : Use WeakPtr for autofill driver. #Patch Set 12 : Rebase. #Patch Set 13 : Rebase. #
Total comments: 6
Patch Set 14 : Addresses Sylvain's review comments. #
Total comments: 27
Patch Set 15 : Addresses Peter's review comments. #Patch Set 16 : Addresses Peter's input over use of WeakPtr<>. #
Total comments: 12
Patch Set 17 : Addresses Peter's review comments. #Patch Set 18 : Remove infobar if its associated page content is destroyed. #
Total comments: 7
Patch Set 19 : Abstracting the call to WebContents::OpenURL() through the AutofillClient. #
Total comments: 10
Patch Set 20 : Addresses Ilya's review inputs. #
Total comments: 6
Patch Set 21 : Addresses Peter's review inputs. #Patch Set 22 : Fixed breakages on Android port. #Messages
Total messages: 104 (15 generated)
pritam.nikam@samsung.com changed reviewers: + estade@chromium.org, isherman@chromium.org
Hi Ilya & Evan, PTAL, Thanks!
isherman@chromium.org changed reviewers: + dconnelly@chromium.org, pkasting@chromium.org
+pkasting to comment on this from an infobars viewpoint -- is this CL appropriate, or should the InfoBarService class be componentized first, so that this infobar delegate doesn't need to manage its own WebContents? +dconnelly to comment on this from an Autofill-on-iOS viewpoint -- is it appropriate to componentize this class so that it lives in the content directory, or would it be better to componentize it further, so that it lives in the core directory, and hence so that more code can be shared with iOS? https://codereview.chromium.org/710453002/diff/40001/chrome/browser/autofill/... File chrome/browser/autofill/autofill_cc_infobar_delegate_unittest.cc (right): https://codereview.chromium.org/710453002/diff/40001/chrome/browser/autofill/... chrome/browser/autofill/autofill_cc_infobar_delegate_unittest.cc:5: #include "components/autofill/content/browser/autofill_cc_infobar_delegate.h" Can this entire test file be moved into the components directory as well? https://codereview.chromium.org/710453002/diff/40001/components/autofill/cont... File components/autofill/content/browser/autofill_cc_infobar_delegate.cc (right): https://codereview.chromium.org/710453002/diff/40001/components/autofill/cont... components/autofill/content/browser/autofill_cc_infobar_delegate.cc:107: if (web_contents_) { Why can the web_contents_ be null? https://codereview.chromium.org/710453002/diff/40001/components/autofill/cont... File components/autofill/content/browser/autofill_cc_infobar_delegate.h (right): https://codereview.chromium.org/710453002/diff/40001/components/autofill/cont... components/autofill/content/browser/autofill_cc_infobar_delegate.h:68: content::WebContents* web_contents_; It's not clear to me that just caching the web_contents_, and assuming that's safe, is appropriate. I suspect that it would be more appropriate to first componentize the InfoBarService class. https://codereview.chromium.org/710453002/diff/40001/components/resources/aut... File components/resources/autofill_cc_infobar_resources.grdp (right): https://codereview.chromium.org/710453002/diff/40001/components/resources/aut... components/resources/autofill_cc_infobar_resources.grdp:3: <structure type="chrome_scaled_image" name="IDR_INFOBAR_AUTOFILL_CC" file="common/autofill/infobar_autofill_cc.png" /> Hmm, why did you just add this to autofill_scaled_resources.grdp?
pkasting@chromium.org changed reviewers: + blundell@chromium.org, droger@chromium.org
droger and blundell are more qualified than me to comment on the right path to follow for how componentized infobars should attempt navigations.
On 2014/11/19 22:09:57, Peter Kasting wrote: > droger and blundell are more qualified than me to comment on the right path to > follow for how componentized infobars should attempt navigations. This class should be cleaned of //content dependencies and componentized into //components/autofill/core/browser. David is best qualified to comment on the changes that need to occur beyond the work that has already been done in this CL. My guess is that the infobar delegate should take in an AutofillDriver instance rather than the WebContents, and talk to the AutofillDriver for the things that it was previously talking to the WebContents directly for.
On 2014/11/19 21:22:46, Ilya Sherman wrote: > +pkasting to comment on this from an infobars viewpoint -- is this CL > appropriate, or should the InfoBarService class be componentized first, so that > this infobar delegate doesn't need to manage its own WebContents? > > +dconnelly to comment on this from an Autofill-on-iOS viewpoint -- is it > appropriate to componentize this class so that it lives in the content > directory, or would it be better to componentize it further, so that it lives in > the core directory, and hence so that more code can be shared with iOS? I agree with Colin's suggestion about using AutofillDriver. We're using AutofillCCInfoBarDelegate downstream in AutofillClientIOS and moving it to content/ and requiring a WebContents isn't the right thing to do. > > https://codereview.chromium.org/710453002/diff/40001/chrome/browser/autofill/... > File chrome/browser/autofill/autofill_cc_infobar_delegate_unittest.cc (right): > > https://codereview.chromium.org/710453002/diff/40001/chrome/browser/autofill/... > chrome/browser/autofill/autofill_cc_infobar_delegate_unittest.cc:5: #include > "components/autofill/content/browser/autofill_cc_infobar_delegate.h" > Can this entire test file be moved into the components directory as well? > > https://codereview.chromium.org/710453002/diff/40001/components/autofill/cont... > File components/autofill/content/browser/autofill_cc_infobar_delegate.cc > (right): > > https://codereview.chromium.org/710453002/diff/40001/components/autofill/cont... > components/autofill/content/browser/autofill_cc_infobar_delegate.cc:107: if > (web_contents_) { > Why can the web_contents_ be null? > > https://codereview.chromium.org/710453002/diff/40001/components/autofill/cont... > File components/autofill/content/browser/autofill_cc_infobar_delegate.h (right): > > https://codereview.chromium.org/710453002/diff/40001/components/autofill/cont... > components/autofill/content/browser/autofill_cc_infobar_delegate.h:68: > content::WebContents* web_contents_; > It's not clear to me that just caching the web_contents_, and assuming that's > safe, is appropriate. I suspect that it would be more appropriate to first > componentize the InfoBarService class. > > https://codereview.chromium.org/710453002/diff/40001/components/resources/aut... > File components/resources/autofill_cc_infobar_resources.grdp (right): > > https://codereview.chromium.org/710453002/diff/40001/components/resources/aut... > components/resources/autofill_cc_infobar_resources.grdp:3: <structure > type="chrome_scaled_image" name="IDR_INFOBAR_AUTOFILL_CC" > file="common/autofill/infobar_autofill_cc.png" /> > Hmm, why did you just add this to autofill_scaled_resources.grdp?
I'm not familiar with autofill, but using the driver for this is something we did before. You can see for example that TranslateDriver has a OpenUrlInNewTab() method, and how it is implemented in ContentTranslateDriver. https://codereview.chromium.org/710453002/diff/40001/chrome/browser/autofill/... File chrome/browser/autofill/autofill_cc_infobar_delegate_unittest.cc (right): https://codereview.chromium.org/710453002/diff/40001/chrome/browser/autofill/... chrome/browser/autofill/autofill_cc_infobar_delegate_unittest.cc:5: #include "components/autofill/content/browser/autofill_cc_infobar_delegate.h" On 2014/11/19 21:22:46, Ilya Sherman wrote: > Can this entire test file be moved into the components directory as well? This file has a still a lot of //chrome dependency. Moving it is a very good idea, but maybe in a different CL. https://codereview.chromium.org/710453002/diff/40001/components/autofill/cont... File components/autofill/content/browser/autofill_cc_infobar_delegate.h (right): https://codereview.chromium.org/710453002/diff/40001/components/autofill/cont... components/autofill/content/browser/autofill_cc_infobar_delegate.h:68: content::WebContents* web_contents_; On 2014/11/19 21:22:46, Ilya Sherman wrote: > It's not clear to me that just caching the web_contents_, and assuming that's > safe, is appropriate. I suspect that it would be more appropriate to first > componentize the InfoBarService class. InfoBarService is not meant to be componentized. Using InfoBarManager is the right thing to do here.
Thanks all for pitching in and helping me with review inputs. As per suggestions, I've abstracted the call to WebContents::OpenURL() throuh the AutofillDriver. Patch #4 addresses the same, PTAL. Thanks! https://codereview.chromium.org/710453002/diff/40001/chrome/browser/autofill/... File chrome/browser/autofill/autofill_cc_infobar_delegate_unittest.cc (right): https://codereview.chromium.org/710453002/diff/40001/chrome/browser/autofill/... chrome/browser/autofill/autofill_cc_infobar_delegate_unittest.cc:5: #include "components/autofill/content/browser/autofill_cc_infobar_delegate.h" On 2014/11/20 13:18:07, droger wrote: > On 2014/11/19 21:22:46, Ilya Sherman wrote: > > Can this entire test file be moved into the components directory as well? > > This file has a still a lot of //chrome dependency. > Moving it is a very good idea, but maybe in a different CL. I'll be doing this in seperate CL. As it has dependencies on |/chrome|, after these dependencies get componentize I'll do this CL. https://codereview.chromium.org/710453002/diff/40001/components/autofill/cont... File components/autofill/content/browser/autofill_cc_infobar_delegate.cc (right): https://codereview.chromium.org/710453002/diff/40001/components/autofill/cont... components/autofill/content/browser/autofill_cc_infobar_delegate.cc:107: if (web_contents_) { On 2014/11/19 21:22:46, Ilya Sherman wrote: > Why can the web_contents_ be null? Done. https://codereview.chromium.org/710453002/diff/40001/components/autofill/cont... File components/autofill/content/browser/autofill_cc_infobar_delegate.h (right): https://codereview.chromium.org/710453002/diff/40001/components/autofill/cont... components/autofill/content/browser/autofill_cc_infobar_delegate.h:68: content::WebContents* web_contents_; On 2014/11/20 13:18:08, droger wrote: > On 2014/11/19 21:22:46, Ilya Sherman wrote: > > It's not clear to me that just caching the web_contents_, and assuming that's > > safe, is appropriate. I suspect that it would be more appropriate to first > > componentize the InfoBarService class. > > InfoBarService is not meant to be componentized. > Using InfoBarManager is the right thing to do here. As suggested, abstracting the call to WebContents::OpenURL() throuh the AutofillDriver, which has been passed through to the infobar delegate. https://codereview.chromium.org/710453002/diff/40001/components/resources/aut... File components/resources/autofill_cc_infobar_resources.grdp (right): https://codereview.chromium.org/710453002/diff/40001/components/resources/aut... components/resources/autofill_cc_infobar_resources.grdp:3: <structure type="chrome_scaled_image" name="IDR_INFOBAR_AUTOFILL_CC" file="common/autofill/infobar_autofill_cc.png" /> On 2014/11/19 21:22:46, Ilya Sherman wrote: > Hmm, why did you just add this to autofill_scaled_resources.grdp? Done.
Awesome, thanks! Not going to do a full review of the CL, but one comment: now you should move the AutofillCCInfoBarDelegate into //components/autofill/core/browser so that it's usable from iOS as well. It looks like you cleaned it of all the //content dependencies.
On 2014/11/21 08:38:24, blundell wrote: > Awesome, thanks! Not going to do a full review of the CL, but one comment: now > you should move the AutofillCCInfoBarDelegate into > //components/autofill/core/browser so that it's usable from iOS as well. It > looks like you cleaned it of all the //content dependencies. Thanks blundell! I've tried this by moving AutofillCCInfoBarDelegate under //components/autofill/core/browser, but to my surprise it gives loads of linker warnings/errors. //--- [snap] ---// ld.gold: warning: hidden symbol 'XYZ' in obj/components/libinfobars_core.a(obj/components/infobars/core/infobars_core.infobar_delegate.o) is referenced by DSO lib/libpolicy_component.so I'll try to resolve these, and will update. Thanks!
On 2014/11/21 09:04:49, Pritam Nikam wrote: > On 2014/11/21 08:38:24, blundell wrote: > > Awesome, thanks! Not going to do a full review of the CL, but one comment: now > > you should move the AutofillCCInfoBarDelegate into > > //components/autofill/core/browser so that it's usable from iOS as well. It > > looks like you cleaned it of all the //content dependencies. > > Thanks blundell! > > I've tried this by moving AutofillCCInfoBarDelegate under > //components/autofill/core/browser, but to my surprise it gives loads of linker > warnings/errors. > > //--- [snap] ---// > ld.gold: warning: hidden symbol 'XYZ' in > obj/components/libinfobars_core.a(obj/components/infobars/core/infobars_core.infobar_delegate.o) > is referenced by DSO lib/libpolicy_component.so > > I'll try to resolve these, and will update. > > Thanks! Did you add a dependency on infobars_core in autofill.gypi?
On 2014/11/21 09:15:23, droger wrote: > On 2014/11/21 09:04:49, Pritam Nikam wrote: > > On 2014/11/21 08:38:24, blundell wrote: > > > Awesome, thanks! Not going to do a full review of the CL, but one comment: > now > > > you should move the AutofillCCInfoBarDelegate into > > > //components/autofill/core/browser so that it's usable from iOS as well. It > > > looks like you cleaned it of all the //content dependencies. > > > > Thanks blundell! > > > > I've tried this by moving AutofillCCInfoBarDelegate under > > //components/autofill/core/browser, but to my surprise it gives loads of > linker > > warnings/errors. > > > > //--- [snap] ---// > > ld.gold: warning: hidden symbol 'XYZ' in > > > obj/components/libinfobars_core.a(obj/components/infobars/core/infobars_core.infobar_delegate.o) > > is referenced by DSO lib/libpolicy_component.so > > > > I'll try to resolve these, and will update. > > > > Thanks! > > Did you add a dependency on infobars_core in autofill.gypi? Thanks droger! I've added this in dependencies. However, it has libbrowser_ui.a dependencies (chrome/chrome.gyp), that does not seem quite right too. Thanks!
https://codereview.chromium.org/710453002/diff/60001/components/autofill/cont... File components/autofill/content/browser/content_autofill_driver.cc (right): https://codereview.chromium.org/710453002/diff/60001/components/autofill/cont... components/autofill/content/browser/content_autofill_driver.cc:238: web_contents()->OpenURL(content::OpenURLParams( I don't believe that OpenURL is a const method, so this entire function should not be const. https://codereview.chromium.org/710453002/diff/60001/components/autofill/cont... components/autofill/content/browser/content_autofill_driver.cc:241: ui::PAGE_TRANSITION_LINK, false)); I think some of this work belongs in the infobar class. I'd implement this method as void ContentAutofillDriver::LinkClicked(const GURL& url, WindowOpenDisposition disposition) { web_contents()->OpenURL(content::OpenURLParams( url, content::Referrer(), disposition, ui::PAGE_TRANSITION_LINK, false)); } The remaining code -- i.e. the url constant and the massaging of the disposition -- belong in the infobar class. https://codereview.chromium.org/710453002/diff/60001/components/autofill/core... File components/autofill/core/browser/autofill_client.h (right): https://codereview.chromium.org/710453002/diff/60001/components/autofill/core... components/autofill/core/browser/autofill_client.h:75: const AutofillDriver& autofill_driver, Please pass this as a pointer, since you plan to keep a reference to it. (Also, as I mention in another file, I don't think the "const" is correct.) https://codereview.chromium.org/710453002/diff/60001/components/autofill/core... components/autofill/core/browser/autofill_client.h:75: const AutofillDriver& autofill_driver, Please document the |autofill_driver|, including lifetime expectations. https://codereview.chromium.org/710453002/diff/60001/components/autofill/core... File components/autofill/core/browser/autofill_driver.h (right): https://codereview.chromium.org/710453002/diff/60001/components/autofill/core... components/autofill/core/browser/autofill_driver.h:11: #include "ui/base/window_open_disposition.h" You'll need to add "+ui" to the DEPS file to account for this header inclusion. Likewise, you'll need to update the list of dependencies for both the GYP and the GN build. https://codereview.chromium.org/710453002/diff/60001/components/autofill/core... components/autofill/core/browser/autofill_driver.h:90: virtual void LinkClicked(WindowOpenDisposition disposition) const = 0; This is either too general a method signature, or too vague a name -- it gives no indication that the link that should be opened is specifically the help URL. https://codereview.chromium.org/710453002/diff/60001/components/autofill/core... File components/autofill/core/browser/test_autofill_client.cc (right): https://codereview.chromium.org/710453002/diff/60001/components/autofill/core... components/autofill/core/browser/test_autofill_client.cc:35: const AutofillDriver& autofill_driver, Note: Since the value is never dereferenced, there's no need to ever #include the header file for it. https://codereview.chromium.org/710453002/diff/60001/components/autofill/core... File components/autofill/core/browser/test_autofill_client.h (right): https://codereview.chromium.org/710453002/diff/60001/components/autofill/core... components/autofill/core/browser/test_autofill_client.h:13: #include "components/autofill/core/browser/autofill_driver.h" nit: Please forward-declare (or omit entirely, since the only use is in an overridden method).
LGTM on the _infobar_delegate.* files. https://codereview.chromium.org/710453002/diff/60001/components/autofill/cont... File components/autofill/content/browser/autofill_cc_infobar_delegate.cc (right): https://codereview.chromium.org/710453002/diff/60001/components/autofill/cont... components/autofill/content/browser/autofill_cc_infobar_delegate.cc:85: : IDS_AUTOFILL_CC_INFOBAR_DENY); Nit: Please leave this formatting as it was before (which was not only acceptable but arguably more correct). https://codereview.chromium.org/710453002/diff/60001/components/autofill/cont... File components/autofill/content/browser/autofill_cc_infobar_delegate.h (right): https://codereview.chromium.org/710453002/diff/60001/components/autofill/cont... components/autofill/content/browser/autofill_cc_infobar_delegate.h:66: // Delegates the link clicked action performed on this infobar. Nit: How about "Performs navigations to handle any link clicks." "Delegates the action" confuses me. https://codereview.chromium.org/710453002/diff/60001/components/autofill/cont... File components/autofill/content/browser/content_autofill_driver.cc (right): https://codereview.chromium.org/710453002/diff/60001/components/autofill/cont... components/autofill/content/browser/content_autofill_driver.cc:238: web_contents()->OpenURL(content::OpenURLParams( On 2014/11/21 18:41:52, Ilya Sherman wrote: > I don't believe that OpenURL is a const method, so this entire function should > not be const. I agree, but the reason this was even possible was that content::WebContentsObserver has "WebContents* web_contents() const", which shouldn't be a const function either. It would be nice to fix this as well, but that may be beyond the scope of this change.
Thanks Ilya & Peter for review. I've addressed your inputs in patch set #5, PTAL! However, |components/autofill/content/browser/autofill_cc_infobar_delegate.h/cc| still need to be moved under |components/autofill/core/browser/|. Build ends up with linker warnings/errors for dependencies: a. obj/chrome/libbrowser_ui.a - yet to resolve. b. obj/components/libinfobars_core.a - resolved. c. obj/components/libinfobars_test_support.a - resolved. I'll address these soon and add a patch for review. Thanks! https://codereview.chromium.org/710453002/diff/60001/components/autofill/cont... File components/autofill/content/browser/autofill_cc_infobar_delegate.cc (right): https://codereview.chromium.org/710453002/diff/60001/components/autofill/cont... components/autofill/content/browser/autofill_cc_infobar_delegate.cc:85: : IDS_AUTOFILL_CC_INFOBAR_DENY); On 2014/11/21 19:42:17, Peter Kasting wrote: > Nit: Please leave this formatting as it was before (which was not only > acceptable but arguably more correct). Done. "git cl format" would have disturbed the formating. Restored it back. https://codereview.chromium.org/710453002/diff/60001/components/autofill/cont... File components/autofill/content/browser/autofill_cc_infobar_delegate.h (right): https://codereview.chromium.org/710453002/diff/60001/components/autofill/cont... components/autofill/content/browser/autofill_cc_infobar_delegate.h:66: // Delegates the link clicked action performed on this infobar. On 2014/11/21 19:42:17, Peter Kasting wrote: > Nit: How about "Performs navigations to handle any link clicks." "Delegates the > action" confuses me. Done. https://codereview.chromium.org/710453002/diff/60001/components/autofill/cont... File components/autofill/content/browser/content_autofill_driver.cc (right): https://codereview.chromium.org/710453002/diff/60001/components/autofill/cont... components/autofill/content/browser/content_autofill_driver.cc:241: ui::PAGE_TRANSITION_LINK, false)); On 2014/11/21 18:41:52, Ilya Sherman wrote: > I think some of this work belongs in the infobar class. I'd implement this > method as > > void ContentAutofillDriver::LinkClicked(const GURL& url, > WindowOpenDisposition disposition) { > web_contents()->OpenURL(content::OpenURLParams( > url, content::Referrer(), disposition, ui::PAGE_TRANSITION_LINK, false)); > } > > The remaining code -- i.e. the url constant and the massaging of the disposition > -- belong in the infobar class. Done. https://codereview.chromium.org/710453002/diff/60001/components/autofill/core... File components/autofill/core/browser/autofill_client.h (right): https://codereview.chromium.org/710453002/diff/60001/components/autofill/core... components/autofill/core/browser/autofill_client.h:75: const AutofillDriver& autofill_driver, On 2014/11/21 18:41:53, Ilya Sherman wrote: > Please pass this as a pointer, since you plan to keep a reference to it. (Also, > as I mention in another file, I don't think the "const" is correct.) Done. https://codereview.chromium.org/710453002/diff/60001/components/autofill/core... File components/autofill/core/browser/autofill_driver.h (right): https://codereview.chromium.org/710453002/diff/60001/components/autofill/core... components/autofill/core/browser/autofill_driver.h:11: #include "ui/base/window_open_disposition.h" On 2014/11/21 18:41:53, Ilya Sherman wrote: > You'll need to add "+ui" to the DEPS file to account for this header inclusion. > Likewise, you'll need to update the list of dependencies for both the GYP and > the GN build. Done. Added dependency under |components/autofill/core/browser/DEPS|. For autofill.gypi I could see existing |'../ui/base/ui_base.gyp:ui_base'| under dependency tree, same with the GN build |//ui/base|. https://codereview.chromium.org/710453002/diff/60001/components/autofill/core... components/autofill/core/browser/autofill_driver.h:90: virtual void LinkClicked(WindowOpenDisposition disposition) const = 0; On 2014/11/21 18:41:53, Ilya Sherman wrote: > This is either too general a method signature, or too vague a name -- it gives > no indication that the link that should be opened is specifically the help URL. Done. Changed to: virtual void LinkClicked(const GURL& url, WindowOpenDisposition disposition) = 0; https://codereview.chromium.org/710453002/diff/60001/components/autofill/core... File components/autofill/core/browser/test_autofill_client.cc (right): https://codereview.chromium.org/710453002/diff/60001/components/autofill/core... components/autofill/core/browser/test_autofill_client.cc:35: const AutofillDriver& autofill_driver, On 2014/11/21 18:41:53, Ilya Sherman wrote: > Note: Since the value is never dereferenced, there's no need to ever #include > the header file for it. Done. https://codereview.chromium.org/710453002/diff/60001/components/autofill/core... File components/autofill/core/browser/test_autofill_client.h (right): https://codereview.chromium.org/710453002/diff/60001/components/autofill/core... components/autofill/core/browser/test_autofill_client.h:13: #include "components/autofill/core/browser/autofill_driver.h" On 2014/11/21 18:41:53, Ilya Sherman wrote: > nit: Please forward-declare (or omit entirely, since the only use is in an > overridden method). Done.
Hi All, Sorry for the updates on this CL. Please have a look at patch set #6. It addresses moving |autofill_cc_infobar_delegate.cc/h| to |components/autofill/core/browser|. I've tried solving linker warnings and have some code restructuring related to the same. Thanks!
I'm skeptical about changes made for Static constants defined in |infobar.h|, for now I've added these under |components/infobars/core/infobar_constants.cc|. Please let me know what shall be the right way solve it. Without this change build has linker errors. Thanks!
On 2014/11/28 11:30:25, Pritam Nikam wrote: > I'm skeptical about changes made for Static constants defined in |infobar.h|, > for now I've added these under |components/infobars/core/infobar_constants.cc|. > Please let me know what shall be the right way solve it. Without this change > build has linker errors. What errors? I don't see why non-Android builds should be requiring these constants here, as they have real definitions for them elsewhere. If anything, defining these ought to break the build for e.g. views.
On 2014/11/29 19:50:01, Peter Kasting wrote: > On 2014/11/28 11:30:25, Pritam Nikam wrote: > > I'm skeptical about changes made for Static constants defined in |infobar.h|, > > for now I've added these under > |components/infobars/core/infobar_constants.cc|. > > Please let me know what shall be the right way solve it. Without this change > > build has linker errors. > > What errors? I don't see why non-Android builds should be requiring these > constants here, as they have real definitions for them elsewhere. If anything, > defining these ought to break the build for e.g. views. Thanks Peter. I'm experimenting with views builds only, and without |components/infobars/core/infobar_constants.cc| changes I'm getting below linker errors: // --- [snap] --- // $ build/gyp_chromium -D component=shared_library $ ninja -j16 -C out/Debug chrome Linker errors: ../src/third_party/binutils/Linux_x64/Release/bin/ld.gold: warning: hidden symbol 'infobars::InfoBar::kSeparatorLineHeight' in obj/chrome/libbrowser_ui.a(obj/chrome/browser/ui/views/infobars/browser_ui.infobar_view.o) is referenced by DSO lib/libpolicy_component.so ... And these errors are for rest of the infobar constants as well. Moreover, If I do not provide *-D component=shared_library* option to $build/gyp_chromium, it builds without a problem. Thanks!
On 2014/12/01 05:49:04, Pritam Nikam wrote: > On 2014/11/29 19:50:01, Peter Kasting wrote: > > On 2014/11/28 11:30:25, Pritam Nikam wrote: > > > I'm skeptical about changes made for Static constants defined in > |infobar.h|, > > > for now I've added these under > > |components/infobars/core/infobar_constants.cc|. > > > Please let me know what shall be the right way solve it. Without this change > > > build has linker errors. > > > > What errors? I don't see why non-Android builds should be requiring these > > constants here, as they have real definitions for them elsewhere. If > anything, > > defining these ought to break the build for e.g. views. > > Thanks Peter. > > I'm experimenting with views builds only, and without > |components/infobars/core/infobar_constants.cc| changes I'm getting below linker > errors: > > // --- [snap] --- // > $ build/gyp_chromium -D component=shared_library > $ ninja -j16 -C out/Debug chrome > > Linker errors: > ../src/third_party/binutils/Linux_x64/Release/bin/ld.gold: warning: hidden > symbol 'infobars::InfoBar::kSeparatorLineHeight' in > obj/chrome/libbrowser_ui.a(obj/chrome/browser/ui/views/infobars/browser_ui.infobar_view.o) > is referenced by DSO lib/libpolicy_component.so > > ... > > And these errors are for rest of the infobar constants as well. This is because you've instantiated InfoBar directly in test_autofill_client.cc. That's not correct; this class is an incomplete base intended to be fully-implemented in chrome-side code, because it represents the "view" of the infobar rather than its model. Why did you change to providing a CreateInfoBar() method on the AutofillClient instead of invoking ConfirmInfoBarDelegate::CreateInfoBar() directly in AutofillCCInfoBarDelegate::Create()? I didn't pay enough attention to that change, and that's what's causing this. I would try to revert that change if possible; I believe that should fix your linker errors. I didn't see a reason in the thread above to require this indirection, but perhaps I missed something. If there's truly a reason why this abstraction has to exist, can the test code's implementation simply return nullptr? It doesn't seem like the tests should be doing something that requires a non-null pointer but works with an instantiation of the base InfoBar class instead of the "real" implementations on the chrome side.
Thanks Peter for your inputs. > > This is because you've instantiated InfoBar directly in test_autofill_client.cc. > That's not correct; this class is an incomplete base intended to be > fully-implemented in chrome-side code, because it represents the "view" of the > infobar rather than its model. > > Why did you change to providing a CreateInfoBar() method on the AutofillClient > instead of invoking ConfirmInfoBarDelegate::CreateInfoBar() directly in > AutofillCCInfoBarDelegate::Create()? I didn't pay enough attention to that > change, and that's what's causing this. I would try to revert that change if > possible; I believe that should fix your linker errors. I didn't see a reason > in the thread above to require this indirection, but perhaps I missed something. I've abstracted this to resolve linker error (with *-D component=shared_library*), in a way to remove dependencies on |chrome/browser_ui|. The definition for this static function is under, |chrome/browser/ui/views/infobars/confirm_infobar.cc| for views (and respective confirm_infobar.cc on other ports). I believe this abstraction will help us to get rid of these dependencies, and caller in component (AutofillCCInfoBarDelegate in our case) can happily do CreateInfoBar() via this abstraction without warring about actual chrome-side implementation. |test_autofill_client.cc| changes are just to override this interface. > > If there's truly a reason why this abstraction has to exist, can the test code's > implementation simply return nullptr? It doesn't seem like the tests should be > doing something that requires a non-null pointer but works with an instantiation > of the base InfoBar class instead of the "real" implementations on the chrome > side. I'll return nullptr instead, of-course if abstraction needs to be there. Thanks!
Hi Peter, I've tried reverting the CreateInfoBar() method from the AutofillClient interface again, but I could see the linker errors. For rest of the linker errors for infobar constants, with newest patch set (#7) i've included it conditionally, i.e. only when options *-D component=shared_library* is provided. PTAL! Thanks!
On 2014/12/02 14:12:40, Pritam Nikam wrote: > Hi Peter, > > I've tried reverting the CreateInfoBar() method from the AutofillClient > interface again, but I could see the linker errors. > For rest of the linker errors for infobar constants, with newest patch set (#7) > i've included it conditionally, i.e. only when options *-D > component=shared_library* is provided. PTAL! > > Thanks! Sorry, could you describe again what problems led you to add AutofillClient::CreateInfoBar()? That seems suspicious. What do you mean by "included it conditionally"?
>
> Sorry, could you describe again what problems led you to add
> AutofillClient::CreateInfoBar()? That seems suspicious.
Hi Blundell,
After I moved files |autofill_cc_infobar_delegate.h/cc| under
|components/autofill/core/browser| folder, I was getting linker errors for:
1. |ConfirmInfoBarDelegate::CreateInfoBar()| this symbol, was showing dependency
on |chrome/browser_ui| (/chrome/libbrowser_ui.a).
-- To resolve this dependency I've added an abstraction i.e. pure
virtual function in AutofillClient.
2. static const declared under infobar.h
(infobars::InfoBar::kSeparatorLineHeight, etc...).
-- This was happening as I was using build option *-D
component=shared_library*, in my local build setup.
e.g.
$ build/gyp_chromium -D component=shared_library
$ ninja -j16 -C out/Debug chrome
something like below linker errors:
../src/third_party/binutils/Linux_x64/Release/bin/ld.gold: warning: hidden
symbol 'infobars::InfoBar::kSeparatorLineHeight' in
obj/chrome/libbrowser_ui.a(obj/chrome/browser/ui/views/infobars/browser_ui.infobar_view.o)
is referenced by DSO lib/libpolicy_component.so
>
> What do you mean by "included it conditionally"?
To resolve, I've added definitions under new file
|components/infobars/core/infobar_constants.cc|, and under infobars.gypi added a
conditional inclusion (below code snippet) so that it will break other builds
without -D component=shared_library option.
'conditions': [
['component=="shared_library"', {
'sources': [
'infobars/core/infobar_constants.cc',
],
}],
],
[This approach does not seems right, as it's documented in infobar.h,
definitions are port specific & must be provided by chrome-side implementation].
Thanks!
On 2014/12/02 14:39:05, Pritam Nikam wrote:
> >
> > Sorry, could you describe again what problems led you to add
> > AutofillClient::CreateInfoBar()? That seems suspicious.
>
>
> Hi Blundell,
>
> After I moved files |autofill_cc_infobar_delegate.h/cc| under
> |components/autofill/core/browser| folder, I was getting linker errors for:
>
> 1. |ConfirmInfoBarDelegate::CreateInfoBar()| this symbol, was showing
dependency
> on |chrome/browser_ui| (/chrome/libbrowser_ui.a).
> -- To resolve this dependency I've added an abstraction i.e. pure
> virtual function in AutofillClient.
>
> 2. static const declared under infobar.h
> (infobars::InfoBar::kSeparatorLineHeight, etc...).
> -- This was happening as I was using build option *-D
> component=shared_library*, in my local build setup.
> e.g.
> $ build/gyp_chromium -D component=shared_library
> $ ninja -j16 -C out/Debug chrome
>
> something like below linker errors:
>
> ../src/third_party/binutils/Linux_x64/Release/bin/ld.gold: warning: hidden
> symbol 'infobars::InfoBar::kSeparatorLineHeight' in
>
obj/chrome/libbrowser_ui.a(obj/chrome/browser/ui/views/infobars/browser_ui.infobar_view.o)
> is referenced by DSO lib/libpolicy_component.so
>
>
> >
> > What do you mean by "included it conditionally"?
>
>
> To resolve, I've added definitions under new file
> |components/infobars/core/infobar_constants.cc|, and under infobars.gypi added
a
> conditional inclusion (below code snippet) so that it will break other builds
> without -D component=shared_library option.
>
> 'conditions': [
> ['component=="shared_library"', {
> 'sources': [
> 'infobars/core/infobar_constants.cc',
> ],
> }],
> ],
>
> [This approach does not seems right, as it's documented in infobar.h,
> definitions are port specific & must be provided by chrome-side
implementation].
>
> Thanks!
Which targets were you getting these build errors on?
> Which targets were you getting these build errors on? For Linux (x64).
On 2014/12/02 15:53:16, Pritam Nikam wrote: > > Which targets were you getting these build errors on? > > For Linux (x64). Sorry, I mean which targets were failing to link?
On 2014/12/02 15:54:06, blundell wrote: > On 2014/12/02 15:53:16, Pritam Nikam wrote: > > > Which targets were you getting these build errors on? > > > > For Linux (x64). > > Sorry, I mean which targets were failing to link? I tried with "ninja -C out/Debug chrome" linker error appears while linking "components/policy_component (linking lib/libpolicy_component.so)".
blundell@chromium.org changed reviewers: + sdefresne@chromium.org
:(. The solution in infobars.gypi isn't going to fly. Sylvain/David, do you have any suggestions for a workaround, or do we just need to bite the bullet and fix this problem of the infobar component's statics?
On 2014/12/02 16:32:36, blundell wrote: > :(. The solution in infobars.gypi isn't going to fly. Sylvain/David, do you have > any suggestions for a workaround, or do we just need to bite the bullet and fix > this problem of the infobar component's statics? I don't understand why patch set 7 is giving linker errors at all. Why is the policy component suddenly supposedly depending on the infobar constants? We have two other componentized infobar delegates, TranslateInfoBarDelegate and GoogleURLTrackerInfoBarDelegate. The first uses a TranslateClient method to create the infobar, which is pure-virtual and only implemented on the chrome side. That's somewhat like this patch but without the components-side "test" implementation. The second just calls ConfirmInfoBarDelegate::CreateInfoBar() directly, which means it depends on a symbol that's only implemented chrome-side. I don't know how that doesn't trigger its own set of linker errors. All of this smells really strange. I think the key here is to understand why those other two classes don't cause problems of their own.
pritam.nikam: can you try patching https://codereview.chromium.org/752853005/ and your CL in another client and see if it solve the linker errors?
On 2014/12/03 13:37:57, sdefresne wrote: > pritam.nikam: can you try patching https://codereview.chromium.org/752853005/ > and your CL in another client and see if it solve the linker errors? Thanks sdefresne! Patching my changes atop https://codereview.chromium.org/752853005/, *does not* throw any linker error. I've tried build with below targets, $ build/gyp_chromium -D component=shared_library $ ninja -j16 -C out/Debug chrome Tried for all targets as well. $ ninja -j16 -C out/Debug all Thanks!
Thanks, Pritam. Some more comments your way: https://codereview.chromium.org/710453002/diff/120001/chrome/browser/ui/autof... File chrome/browser/ui/autofill/chrome_autofill_client.cc (right): https://codereview.chromium.org/710453002/diff/120001/chrome/browser/ui/autof... chrome/browser/ui/autofill/chrome_autofill_client.cc:116: } Does this entire method still need to be defined on the client, or could we reduce the client's work to just the following? infobars::InfoBarManager* GetInfoBarManager() { return InfoBarService::FromWebContents(web_contents_); } https://codereview.chromium.org/710453002/diff/120001/chrome/browser/ui/autof... File chrome/browser/ui/autofill/chrome_autofill_client.h (right): https://codereview.chromium.org/710453002/diff/120001/chrome/browser/ui/autof... chrome/browser/ui/autofill/chrome_autofill_client.h:33: class AutofillManager; nit: Alphabetize, please. https://codereview.chromium.org/710453002/diff/120001/components/autofill.gypi File components/autofill.gypi (right): https://codereview.chromium.org/710453002/diff/120001/components/autofill.gyp... components/autofill.gypi:360: 'infobars_core', Why is this needed? https://codereview.chromium.org/710453002/diff/120001/components/autofill.gyp... components/autofill.gypi:428: 'infobars_core', Why does the renderer need a dependency on infobars? https://codereview.chromium.org/710453002/diff/120001/components/autofill/cor... File components/autofill/core/browser/BUILD.gn (right): https://codereview.chromium.org/710453002/diff/120001/components/autofill/cor... components/autofill/core/browser/BUILD.gn:160: ] Please add the same deps here as in the gyp file. https://codereview.chromium.org/710453002/diff/120001/components/autofill/cor... File components/autofill/core/browser/DEPS (right): https://codereview.chromium.org/710453002/diff/120001/components/autofill/cor... components/autofill/core/browser/DEPS:15: "+ui", Can this rule be more specific, i.e. just be ui/base? https://codereview.chromium.org/710453002/diff/120001/components/autofill/cor... File components/autofill/core/browser/autofill_cc_infobar_delegate.cc (right): https://codereview.chromium.org/710453002/diff/120001/components/autofill/cor... components/autofill/core/browser/autofill_cc_infobar_delegate.cc:25: AutofillManager* autofill_manager, Can you pass the driver directly, rather than passing the manager? https://codereview.chromium.org/710453002/diff/120001/components/autofill/cor... components/autofill/core/browser/autofill_cc_infobar_delegate.cc:86: IDS_AUTOFILL_CC_INFOBAR_ACCEPT : IDS_AUTOFILL_CC_INFOBAR_DENY); Hmm, this formatting change looks wrong -- is this coming from git cl format? https://codereview.chromium.org/710453002/diff/120001/components/autofill/cor... components/autofill/core/browser/autofill_cc_infobar_delegate.cc:106: autofill_driver_->LinkClicked(GURL(autofill::kHelpURL), disposition); You've dropped the code that sets the disposition. Please restore it. https://codereview.chromium.org/710453002/diff/120001/components/autofill/cor... File components/autofill/core/browser/autofill_cc_infobar_delegate.h (right): https://codereview.chromium.org/710453002/diff/120001/components/autofill/cor... components/autofill/core/browser/autofill_cc_infobar_delegate.h:68: AutofillDriver* autofill_driver_; Please document lifetime expectations. Are you confident that the driver outlives the infobar? https://codereview.chromium.org/710453002/diff/120001/components/autofill/cor... File components/autofill/core/browser/autofill_client.h (right): https://codereview.chromium.org/710453002/diff/120001/components/autofill/cor... components/autofill/core/browser/autofill_client.h:141: // Returns a translate infobar that owns |delegate|. Why a translate infobar? https://codereview.chromium.org/710453002/diff/120001/components/autofill/cor... components/autofill/core/browser/autofill_client.h:143: scoped_ptr<ConfirmInfoBarDelegate> delegate) = 0; I suspect that this method is not appropriate, but I defer to Peter and others for their knowledge of the infobars component.
Thanks Ilya for review. With patch set #8, I've addressed most of your inputs. PTAL! Thanks! https://codereview.chromium.org/710453002/diff/120001/chrome/browser/ui/autof... File chrome/browser/ui/autofill/chrome_autofill_client.cc (right): https://codereview.chromium.org/710453002/diff/120001/chrome/browser/ui/autof... chrome/browser/ui/autofill/chrome_autofill_client.cc:116: } On 2014/12/03 20:00:20, Ilya Sherman wrote: > Does this entire method still need to be defined on the client, or could we > reduce the client's work to just the following? > > infobars::InfoBarManager* GetInfoBarManager() { > return InfoBarService::FromWebContents(web_contents_); > } Done. But I think, I didn't get this completely. https://codereview.chromium.org/710453002/diff/120001/chrome/browser/ui/autof... File chrome/browser/ui/autofill/chrome_autofill_client.h (right): https://codereview.chromium.org/710453002/diff/120001/chrome/browser/ui/autof... chrome/browser/ui/autofill/chrome_autofill_client.h:33: class AutofillManager; On 2014/12/03 20:00:20, Ilya Sherman wrote: > nit: Alphabetize, please. Done. https://codereview.chromium.org/710453002/diff/120001/components/autofill.gypi File components/autofill.gypi (right): https://codereview.chromium.org/710453002/diff/120001/components/autofill.gyp... components/autofill.gypi:360: 'infobars_core', On 2014/12/03 20:00:20, Ilya Sherman wrote: > Why is this needed? Done. Over-boarded! removed. https://codereview.chromium.org/710453002/diff/120001/components/autofill.gyp... components/autofill.gypi:428: 'infobars_core', On 2014/12/03 20:00:20, Ilya Sherman wrote: > Why does the renderer need a dependency on infobars? Done. Ditto! https://codereview.chromium.org/710453002/diff/120001/components/autofill/cor... File components/autofill/core/browser/BUILD.gn (right): https://codereview.chromium.org/710453002/diff/120001/components/autofill/cor... components/autofill/core/browser/BUILD.gn:160: ] On 2014/12/03 20:00:20, Ilya Sherman wrote: > Please add the same deps here as in the gyp file. Done. https://codereview.chromium.org/710453002/diff/120001/components/autofill/cor... File components/autofill/core/browser/DEPS (right): https://codereview.chromium.org/710453002/diff/120001/components/autofill/cor... components/autofill/core/browser/DEPS:15: "+ui", On 2014/12/03 20:00:20, Ilya Sherman wrote: > Can this rule be more specific, i.e. just be ui/base? Done. https://codereview.chromium.org/710453002/diff/120001/components/autofill/cor... File components/autofill/core/browser/autofill_cc_infobar_delegate.cc (right): https://codereview.chromium.org/710453002/diff/120001/components/autofill/cor... components/autofill/core/browser/autofill_cc_infobar_delegate.cc:25: AutofillManager* autofill_manager, On 2014/12/03 20:00:21, Ilya Sherman wrote: > Can you pass the driver directly, rather than passing the manager? In that case, I have to pass both AutofillClient and AutofillDriver pointers. A. to create infobar -> AutofillClient::CreateInfoBar() B. to perform link click -> AutofillDriver::LinkClicked() https://codereview.chromium.org/710453002/diff/120001/components/autofill/cor... components/autofill/core/browser/autofill_cc_infobar_delegate.cc:86: IDS_AUTOFILL_CC_INFOBAR_ACCEPT : IDS_AUTOFILL_CC_INFOBAR_DENY); On 2014/12/03 20:00:20, Ilya Sherman wrote: > Hmm, this formatting change looks wrong -- is this coming from git cl format? Done. on "git cl format" return l10n_util::GetStringUTF16((button == BUTTON_OK) ? IDS_AUTOFILL_CC_INFOBAR_ACCEPT : IDS_AUTOFILL_CC_INFOBAR_DENY); I was trying to restore it back to original, that might have made it more ugly. Corrected it. https://codereview.chromium.org/710453002/diff/120001/components/autofill/cor... components/autofill/core/browser/autofill_cc_infobar_delegate.cc:106: autofill_driver_->LinkClicked(GURL(autofill::kHelpURL), disposition); On 2014/12/03 20:00:20, Ilya Sherman wrote: > You've dropped the code that sets the disposition. Please restore it. Done. https://codereview.chromium.org/710453002/diff/120001/components/autofill/cor... File components/autofill/core/browser/autofill_cc_infobar_delegate.h (right): https://codereview.chromium.org/710453002/diff/120001/components/autofill/cor... components/autofill/core/browser/autofill_cc_infobar_delegate.h:68: AutofillDriver* autofill_driver_; On 2014/12/03 20:00:21, Ilya Sherman wrote: > Please document lifetime expectations. Are you confident that the driver > outlives the infobar? Done. https://codereview.chromium.org/710453002/diff/120001/components/autofill/cor... File components/autofill/core/browser/autofill_client.h (right): https://codereview.chromium.org/710453002/diff/120001/components/autofill/cor... components/autofill/core/browser/autofill_client.h:141: // Returns a translate infobar that owns |delegate|. On 2014/12/03 20:00:21, Ilya Sherman wrote: > Why a translate infobar? Done.
Let's pause this CL until the issue with infobar constants can be worked out separately.
On 2014/12/04 20:20:30, Peter Kasting wrote: > Let's pause this CL until the issue with infobar constants can be worked out > separately. SGTM. Please let me know when this is ready for another look.
Once https://codereview.chromium.org/793783003/ lands, hopefully this CL can proceed; that ought to fix any need for something like infobar_constants.cc.
On 2014/12/12 01:54:02, Peter Kasting wrote: > Once https://codereview.chromium.org/793783003/ lands, hopefully this CL can > proceed; that ought to fix any need for something like infobar_constants.cc. Rebased atop https://codereview.chromium.org/793783003/, No linkers errors for infobar constants, PTAL. Thanks!
https://codereview.chromium.org/710453002/diff/160001/components/autofill/cor... File components/autofill/core/browser/autofill_client.h (right): https://codereview.chromium.org/710453002/diff/160001/components/autofill/cor... components/autofill/core/browser/autofill_client.h:150: virtual scoped_ptr<infobars::InfoBar> CreateInfoBar( Can't you get rid of this now?
Thanks blundell! I could see the same linker errors if I remove |CreateInfoBar()| from |autofill_client.h|. Thanks! https://codereview.chromium.org/710453002/diff/160001/components/autofill/cor... File components/autofill/core/browser/autofill_client.h (right): https://codereview.chromium.org/710453002/diff/160001/components/autofill/cor... components/autofill/core/browser/autofill_client.h:150: virtual scoped_ptr<infobars::InfoBar> CreateInfoBar( On 2014/12/12 16:03:07, blundell wrote: > Can't you get rid of this now? No. Still the same linker error if I revert this interface. // ---[snap] ---// src/third_party/binutils/Linux_x64/Release/bin/ld.gold: warning: hidden symbol 'ConfirmInfoBarDelegate::CreateInfoBar(scoped_ptr<ConfirmInfoBarDelegate, base::DefaultDeleter<ConfirmInfoBarDelegate> >)' in obj/chrome/libbrowser_ui.a(obj/chrome/browser/ui/views/infobars/browser_ui.confirm_infobar.o) is referenced by DSO lib/libpolicy_component.so
On 2014/12/12 16:19:58, Pritam Nikam wrote: > Thanks blundell! > > I could see the same linker errors if I remove |CreateInfoBar()| from > |autofill_client.h|. > > Thanks! > > https://codereview.chromium.org/710453002/diff/160001/components/autofill/cor... > File components/autofill/core/browser/autofill_client.h (right): > > https://codereview.chromium.org/710453002/diff/160001/components/autofill/cor... > components/autofill/core/browser/autofill_client.h:150: virtual > scoped_ptr<infobars::InfoBar> CreateInfoBar( > On 2014/12/12 16:03:07, blundell wrote: > > Can't you get rid of this now? > > No. Still the same linker error if I revert this interface. > > // ---[snap] ---// > src/third_party/binutils/Linux_x64/Release/bin/ld.gold: warning: hidden symbol > 'ConfirmInfoBarDelegate::CreateInfoBar(scoped_ptr<ConfirmInfoBarDelegate, > base::DefaultDeleter<ConfirmInfoBarDelegate> >)' in > obj/chrome/libbrowser_ui.a(obj/chrome/browser/ui/views/infobars/browser_ui.confirm_infobar.o) > is referenced by DSO lib/libpolicy_component.so oh I see, PK's CL was about the constants but not ConfirmInfoBarDelegate::CreateInfoBar. Carry on.
On 2014/12/12 16:21:29, blundell (OOO until January 5) wrote: > oh I see, PK's CL was about the constants but not > ConfirmInfoBarDelegate::CreateInfoBar. Carry on. Do you have an idea for how we could get rid of this generally? My CL nuked most of src/components/infobars/test/infobar_test.cc, but this confirm infobar issue still causes us to have a function definition there. It would be nice not to have to slowly add individual "create infobar" APIs to all the different pieces of components. It's just ConfirmInfoBarDelegate::CreateInfoBar() that's problematic, so it seems like there ought to be a solution. I suppose one way would be, instead of having this be a static method on ConfirmInfoBarDelegate, have it be pure virtual on some singleton abstract class we expect the embedder to implement. To preserve the existing "only confirm infobar delegate subclasses can do this" restriction, we could make this private, make ConfirmInfoBarDelegate a friend, and keep the existing static method on that class, but implement it in the components/ code to call over to the new abstract method. Doing this would require that such an embedder class actually exist. Not sure if one does.
https://codereview.chromium.org/710453002/diff/160001/chrome/browser/ui/autof... File chrome/browser/ui/autofill/chrome_autofill_client.cc (right): https://codereview.chromium.org/710453002/diff/160001/chrome/browser/ui/autof... chrome/browser/ui/autofill/chrome_autofill_client.cc:132: } Can you remove the AutofillClient::ConfirmSaveCreditCard() method, and just call AutofillClient::GetInfoBarManager() instead? https://codereview.chromium.org/710453002/diff/160001/components/autofill/cor... File components/autofill/core/browser/DEPS (right): https://codereview.chromium.org/710453002/diff/160001/components/autofill/cor... components/autofill/core/browser/DEPS:17: "+ui/gfx/geometry", nit: No need for ui/gfx/geometry if you already allow ui/gfx. https://codereview.chromium.org/710453002/diff/160001/components/autofill/cor... File components/autofill/core/browser/autofill_cc_infobar_delegate.h (right): https://codereview.chromium.org/710453002/diff/160001/components/autofill/cor... components/autofill/core/browser/autofill_cc_infobar_delegate.h:66: // long as it's tab persists. The relevant lifetime expectation is "The driver must outlive the infobar." Given that both are destroyed around the time that the tab is destroyed, what is guaranteeing that the infobar is destroyed first?
Thanks Ilya & Peter for review! I've addressed Iya's inputs with CL #10. PTAL. Thanks! https://codereview.chromium.org/710453002/diff/160001/chrome/browser/ui/autof... File chrome/browser/ui/autofill/chrome_autofill_client.cc (right): https://codereview.chromium.org/710453002/diff/160001/chrome/browser/ui/autof... chrome/browser/ui/autofill/chrome_autofill_client.cc:132: } On 2014/12/12 22:24:21, Ilya Sherman wrote: > Can you remove the AutofillClient::ConfirmSaveCreditCard() method, and just call > AutofillClient::GetInfoBarManager() instead? Done. https://codereview.chromium.org/710453002/diff/160001/components/autofill/cor... File components/autofill/core/browser/DEPS (right): https://codereview.chromium.org/710453002/diff/160001/components/autofill/cor... components/autofill/core/browser/DEPS:17: "+ui/gfx/geometry", On 2014/12/12 22:24:21, Ilya Sherman wrote: > nit: No need for ui/gfx/geometry if you already allow ui/gfx. Done. https://codereview.chromium.org/710453002/diff/160001/components/autofill/cor... File components/autofill/core/browser/autofill_cc_infobar_delegate.h (right): https://codereview.chromium.org/710453002/diff/160001/components/autofill/cor... components/autofill/core/browser/autofill_cc_infobar_delegate.h:66: // long as it's tab persists. On 2014/12/12 22:24:21, Ilya Sherman wrote: > The relevant lifetime expectation is "The driver must outlive the infobar." > Given that both are destroyed around the time that the tab is destroyed, what is > guaranteeing that the infobar is destroyed first? Perhaps, AutofillDriver is destroyed before infobar. Please correct my understanding, AutofillDrivers are per RenderFrame instance within the WebContents, and supposedly get destroyed early. Moreover, code snippet of WebContentsImpl dtor [A] shows that renderer frames destroy (and eventually bringing down AutofillDriver) before it destroys InfoBarDelegate [B]. [A]. https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/we... [B]. https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/we...
https://codereview.chromium.org/710453002/diff/160001/components/autofill/cor... File components/autofill/core/browser/autofill_cc_infobar_delegate.h (right): https://codereview.chromium.org/710453002/diff/160001/components/autofill/cor... components/autofill/core/browser/autofill_cc_infobar_delegate.h:66: // long as it's tab persists. On 2014/12/15 11:16:14, Pritam Nikam wrote: > On 2014/12/12 22:24:21, Ilya Sherman wrote: > > The relevant lifetime expectation is "The driver must outlive the infobar." > > Given that both are destroyed around the time that the tab is destroyed, what > is > > guaranteeing that the infobar is destroyed first? > > Perhaps, AutofillDriver is destroyed before infobar. > > Please correct my understanding, AutofillDrivers are per RenderFrame instance > within the WebContents, and supposedly get destroyed early. Moreover, code > snippet of WebContentsImpl dtor [A] shows that renderer frames destroy (and > eventually bringing down AutofillDriver) before it destroys InfoBarDelegate [B]. > > [A]. > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/we... > > [B]. > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/we... It sounds like you're saying that there's no guarantee that the driver outlives the infobar. In that case, please use a WeakPtr, or find some other way to guarantee that the infobar class cannot attempt to read freed memory.
It'd be really nice to address the issue I raised earlier about how adding CreateInfoBar() wrappers is sort of a hack. I haven't had time yet to experiment with fixes myself, but one possible place we could put the virtual method I mentioned is on the InfoBarManager. I'm not sure whether this is sufficiently non-ugly.
On 2014/12/16 02:58:27, Peter Kasting wrote: > It'd be really nice to address the issue I raised earlier about how adding > CreateInfoBar() wrappers is sort of a hack. I haven't had time yet to > experiment with fixes myself, but one possible place we could put the virtual > method I mentioned is on the InfoBarManager. I'm not sure whether this is > sufficiently non-ugly. Thanks Ilya & Peter for your inputs. I've used WeakPtr<> to address the lifetime expectations of driver withing infobar. PTAL at newest patchset. @ Peter I'm not able to comprehend completly how to abstract |CreateInfoBar()| through InfoBarManager, and embedder provide their own implementations. And as I understand, we are looking for a solution that targets not just AutofillCCInfoBarDelegate & TranslateInfoBarDelegate (and may be other infobar delegates that are yet to componentize). I gave it a try but that didn't work. Thanks!
On 2014/12/18 at 14:10:28, pritam.nikam wrote: > On 2014/12/16 02:58:27, Peter Kasting wrote: > > It'd be really nice to address the issue I raised earlier about how adding > > CreateInfoBar() wrappers is sort of a hack. I haven't had time yet to > > experiment with fixes myself, but one possible place we could put the virtual > > method I mentioned is on the InfoBarManager. I'm not sure whether this is > > sufficiently non-ugly. > > > Thanks Ilya & Peter for your inputs. > > I've used WeakPtr<> to address the lifetime expectations of driver withing infobar. PTAL at newest patchset. > > @ Peter > I'm not able to comprehend completly how to abstract |CreateInfoBar()| through InfoBarManager, and embedder provide their own implementations. And as I understand, we are looking for a solution that targets not just AutofillCCInfoBarDelegate & TranslateInfoBarDelegate (and may be other infobar delegates that are yet to componentize). I gave it a try but that didn't work. > > Thanks! pritam.nikam: I have a CL pending review to revolve the |CreateInfoBar()| issue, see https://codereview.chromium.org/812823002
On 2014/12/18 14:40:22, sdefresne wrote: > On 2014/12/18 at 14:10:28, pritam.nikam wrote: > > On 2014/12/16 02:58:27, Peter Kasting wrote: > > > It'd be really nice to address the issue I raised earlier about how adding > > > CreateInfoBar() wrappers is sort of a hack. I haven't had time yet to > > > experiment with fixes myself, but one possible place we could put the > virtual > > > method I mentioned is on the InfoBarManager. I'm not sure whether this is > > > sufficiently non-ugly. > > > > > > Thanks Ilya & Peter for your inputs. > > > > I've used WeakPtr<> to address the lifetime expectations of driver withing > infobar. PTAL at newest patchset. > > > > @ Peter > > I'm not able to comprehend completly how to abstract |CreateInfoBar()| through > InfoBarManager, and embedder provide their own implementations. And as I > understand, we are looking for a solution that targets not just > AutofillCCInfoBarDelegate & TranslateInfoBarDelegate (and may be other infobar > delegates that are yet to componentize). I gave it a try but that didn't work. > > > > Thanks! > > pritam.nikam: I have a CL pending review to revolve the |CreateInfoBar()| issue, > see https://codereview.chromium.org/812823002 Thanks! Pritam, please let me know once that CL has landed and you've rebased on top of it; I'll then take another look.
On 2014/12/18 23:20:00, Ilya Sherman wrote: > On 2014/12/18 14:40:22, sdefresne wrote: > > On 2014/12/18 at 14:10:28, pritam.nikam wrote: > > > On 2014/12/16 02:58:27, Peter Kasting wrote: > > > > It'd be really nice to address the issue I raised earlier about how adding > > > > CreateInfoBar() wrappers is sort of a hack. I haven't had time yet to > > > > experiment with fixes myself, but one possible place we could put the > > virtual > > > > method I mentioned is on the InfoBarManager. I'm not sure whether this is > > > > sufficiently non-ugly. > > > > > > > > > Thanks Ilya & Peter for your inputs. > > > > > > I've used WeakPtr<> to address the lifetime expectations of driver withing > > infobar. PTAL at newest patchset. > > > > > > @ Peter > > > I'm not able to comprehend completly how to abstract |CreateInfoBar()| > through > > InfoBarManager, and embedder provide their own implementations. And as I > > understand, we are looking for a solution that targets not just > > AutofillCCInfoBarDelegate & TranslateInfoBarDelegate (and may be other infobar > > delegates that are yet to componentize). I gave it a try but that didn't work. > > > > > > Thanks! > > > > pritam.nikam: I have a CL pending review to revolve the |CreateInfoBar()| > issue, > > see https://codereview.chromium.org/812823002 > > Thanks! Pritam, please let me know once that CL has landed and you've rebased > on top of it; I'll then take another look. Thanks Sylvain! Sure, will rebase once Sylvain's patch will be landed. Thanks!
On 2014/12/19 at 09:40:23, pritam.nikam wrote: > On 2014/12/18 23:20:00, Ilya Sherman wrote: > > On 2014/12/18 14:40:22, sdefresne wrote: > > > On 2014/12/18 at 14:10:28, pritam.nikam wrote: > > > > On 2014/12/16 02:58:27, Peter Kasting wrote: > > > > > It'd be really nice to address the issue I raised earlier about how adding > > > > > CreateInfoBar() wrappers is sort of a hack. I haven't had time yet to > > > > > experiment with fixes myself, but one possible place we could put the > > > virtual > > > > > method I mentioned is on the InfoBarManager. I'm not sure whether this is > > > > > sufficiently non-ugly. > > > > > > > > > > > > Thanks Ilya & Peter for your inputs. > > > > > > > > I've used WeakPtr<> to address the lifetime expectations of driver withing > > > infobar. PTAL at newest patchset. > > > > > > > > @ Peter > > > > I'm not able to comprehend completly how to abstract |CreateInfoBar()| > > through > > > InfoBarManager, and embedder provide their own implementations. And as I > > > understand, we are looking for a solution that targets not just > > > AutofillCCInfoBarDelegate & TranslateInfoBarDelegate (and may be other infobar > > > delegates that are yet to componentize). I gave it a try but that didn't work. > > > > > > > > Thanks! > > > > > > pritam.nikam: I have a CL pending review to revolve the |CreateInfoBar()| > > issue, > > > see https://codereview.chromium.org/812823002 > > > > Thanks! Pritam, please let me know once that CL has landed and you've rebased > > on top of it; I'll then take another look. > > Thanks Sylvain! > Sure, will rebase once Sylvain's patch will be landed. > Thanks! Pritam: my patch has landed, hope this will help you land yours :-)
Thanks Sylvain! Dear all, Code re-base is done and patch set #13 is up for review, PTAL. Thanks!
You'll probably also want to edit the change list description since it may now be obsolete (as the ConfirmInfoBarDelegate::CreateInfoBar abstraction now goes through InfoBarManager). https://codereview.chromium.org/710453002/diff/240001/android_webview/native/... File android_webview/native/aw_autofill_client.h (right): https://codereview.chromium.org/710453002/diff/240001/android_webview/native/... android_webview/native/aw_autofill_client.h:92: virtual infobars::InfoBarManager* GetInfoBarManager() override; style: shouldn't you remove the "virtual" there since a method should only have one of "virtual", "override" or "final"? Not sure since this is an android file. https://codereview.chromium.org/710453002/diff/240001/components/autofill/cor... File components/autofill/core/browser/autofill_cc_infobar_delegate.h (right): https://codereview.chromium.org/710453002/diff/240001/components/autofill/cor... components/autofill/core/browser/autofill_cc_infobar_delegate.h:65: // long as it's tab persists. nit: s/it's/its/
Thanks Sylvain! I've addressed your inputs in newest patch set, PTAL, thanks! https://codereview.chromium.org/710453002/diff/240001/android_webview/native/... File android_webview/native/aw_autofill_client.h (right): https://codereview.chromium.org/710453002/diff/240001/android_webview/native/... android_webview/native/aw_autofill_client.h:92: virtual infobars::InfoBarManager* GetInfoBarManager() override; On 2014/12/23 08:43:44, sdefresne wrote: > style: shouldn't you remove the "virtual" there since a method should only have > one of "virtual", "override" or "final"? Not sure since this is an android file. Even I was wondering what to keep here. However, just to keep things align with rest of the virtual function listed in this file, I've kept |virtual| and |override| both. https://codereview.chromium.org/710453002/diff/240001/components/autofill/cor... File components/autofill/core/browser/autofill_cc_infobar_delegate.h (right): https://codereview.chromium.org/710453002/diff/240001/components/autofill/cor... components/autofill/core/browser/autofill_cc_infobar_delegate.h:65: // long as it's tab persists. On 2014/12/23 08:43:44, sdefresne wrote: > nit: s/it's/its/ Done.
https://codereview.chromium.org/710453002/diff/240001/android_webview/native/... File android_webview/native/aw_autofill_client.h (right): https://codereview.chromium.org/710453002/diff/240001/android_webview/native/... android_webview/native/aw_autofill_client.h:92: virtual infobars::InfoBarManager* GetInfoBarManager() override; On 2014/12/23 09:13:35, Pritam Nikam wrote: > On 2014/12/23 08:43:44, sdefresne wrote: > > style: shouldn't you remove the "virtual" there since a method should only > have > > one of "virtual", "override" or "final"? Not sure since this is an android > file. > > Even I was wondering what to keep here. However, just to keep things align with > rest of the virtual function listed in this file, I've kept |virtual| and > |override| both. The correct style in Chromium now is to use only one of the modifiers, so just "override". However, we should be correcting this in an automated fashion over time, so whether you want to bother to change everything in this file now is up to you. I think leaving this as you have it now is fine too. https://codereview.chromium.org/710453002/diff/260001/android_webview/native/... File android_webview/native/aw_autofill_client.h (right): https://codereview.chromium.org/710453002/diff/260001/android_webview/native/... android_webview/native/aw_autofill_client.h:35: } Seems like we shouldn't need this, the base class will have already declared it. https://codereview.chromium.org/710453002/diff/260001/chrome/browser/autofill... File chrome/browser/autofill/autofill_cc_infobar_delegate_unittest.cc (right): https://codereview.chromium.org/710453002/diff/260001/chrome/browser/autofill... chrome/browser/autofill/autofill_cc_infobar_delegate_unittest.cc:59: }; Nit: While here: DISALLOW_COPY_AND_ASSIGN() https://codereview.chromium.org/710453002/diff/260001/chrome/browser/autofill... chrome/browser/autofill/autofill_cc_infobar_delegate_unittest.cc:75: // Associate the |ContentAutofillDriverFactory| to the |WebContents|. Nit: Only use ||s for variable names, not type names. I'm not sure this comment adds much to the code anyway. I might just leave it out. https://codereview.chromium.org/710453002/diff/260001/chrome/browser/autofill... chrome/browser/autofill/autofill_cc_infobar_delegate_unittest.cc:94: Nit: Blank here is unnecessary https://codereview.chromium.org/710453002/diff/260001/chrome/browser/ui/autof... File chrome/browser/ui/autofill/chrome_autofill_client.h (right): https://codereview.chromium.org/710453002/diff/260001/chrome/browser/ui/autof... chrome/browser/ui/autofill/chrome_autofill_client.h:28: } Again, I think we shouldn't need this forward decl. https://codereview.chromium.org/710453002/diff/260001/components/autofill/con... File components/autofill/content/browser/content_autofill_driver.cc (right): https://codereview.chromium.org/710453002/diff/260001/components/autofill/con... components/autofill/content/browser/content_autofill_driver.cc:212: void ContentAutofillDriver::LinkClicked(const GURL& url, The order of functions in the .cc file should match the declaration order in the .h file. https://codereview.chromium.org/710453002/diff/260001/components/autofill/con... File components/autofill/content/browser/content_autofill_driver.h (right): https://codereview.chromium.org/710453002/diff/260001/components/autofill/con... components/autofill/content/browser/content_autofill_driver.h:88: content::WebContents* web_contents(); unix_hacker()-named functions should be simple inlined accessors, otherwise this should be named GetWebContents(). https://codereview.chromium.org/710453002/diff/260001/components/autofill/cor... File components/autofill/core/browser/autofill_cc_infobar_delegate.h (right): https://codereview.chromium.org/710453002/diff/260001/components/autofill/cor... components/autofill/core/browser/autofill_cc_infobar_delegate.h:30: // to |autofill_driver|. This comment isn't right; you don't add infobars to the autofill driver, you add them to the infobar manager. https://codereview.chromium.org/710453002/diff/260001/components/autofill/cor... components/autofill/core/browser/autofill_cc_infobar_delegate.h:66: base::WeakPtr<AutofillDriver> autofill_driver_; If this lasts for the life of the tab, do we really need to hold it as a WeakPtr? Infobars should be torn down as part of the tab destruction order, so I would somewhat expect us to be able to hold this as a raw pointer. Did you verify that's not possible? https://codereview.chromium.org/710453002/diff/260001/components/autofill/cor... File components/autofill/core/browser/autofill_client.h (right): https://codereview.chromium.org/710453002/diff/260001/components/autofill/cor... components/autofill/core/browser/autofill_client.h:136: // Returns the |InfoBarManager| instance associated with the client. Nit: No || https://codereview.chromium.org/710453002/diff/260001/components/autofill/cor... components/autofill/core/browser/autofill_client.h:137: virtual infobars::InfoBarManager* GetInfoBarManager() = 0; Nit: I'd put this up with the other "Gets the X associated with the client" methods at the top of this list. (Also in subclasses) https://codereview.chromium.org/710453002/diff/260001/components/autofill/cor... File components/autofill/core/browser/autofill_driver.h (right): https://codereview.chromium.org/710453002/diff/260001/components/autofill/cor... components/autofill/core/browser/autofill_driver.h:96: // infobar. Nit: How about: Opens |url| with the supplied |disposition|. (This code doesn't really care in what circumstances clients might call it, so it doesn't need to describe that.) https://codereview.chromium.org/710453002/diff/260001/components/autofill/cor... File components/autofill/core/browser/autofill_manager.cc (right): https://codereview.chromium.org/710453002/diff/260001/components/autofill/cor... components/autofill/core/browser/autofill_manager.cc:675: AutofillCCInfoBarDelegate::Create( It seems like the way we abstracted this entire call behind a client method before was a better route. If this is abstracted behind a client call, we don't need to add the GetInfoBarManager() method on the client, because the chrome-side code that implements this can just do that directly to pass the value to AutofillCCInfoBarDelegate::Create(). This also prevents any possible null-deref in test code, where the implementation of GetInfoBarManager() returns null but the infobar delegate doesn't null-check the provided pointer; instead, if this is abstracted, the test implementation of that method can just do nothing.
Thanks Peter for inputs. I've addressed these along new patch set, PTAL. Thanks! https://codereview.chromium.org/710453002/diff/240001/android_webview/native/... File android_webview/native/aw_autofill_client.h (right): https://codereview.chromium.org/710453002/diff/240001/android_webview/native/... android_webview/native/aw_autofill_client.h:92: virtual infobars::InfoBarManager* GetInfoBarManager() override; On 2014/12/23 21:20:34, Peter Kasting wrote: > On 2014/12/23 09:13:35, Pritam Nikam wrote: > > On 2014/12/23 08:43:44, sdefresne wrote: > > > style: shouldn't you remove the "virtual" there since a method should only > > have > > > one of "virtual", "override" or "final"? Not sure since this is an android > > file. > > > > Even I was wondering what to keep here. However, just to keep things align > with > > rest of the virtual function listed in this file, I've kept |virtual| and > > |override| both. > > The correct style in Chromium now is to use only one of the modifiers, so just > "override". > > However, we should be correcting this in an automated fashion over time, so > whether you want to bother to change everything in this file now is up to you. > I think leaving this as you have it now is fine too. I'll keep this as it is for now. https://codereview.chromium.org/710453002/diff/260001/android_webview/native/... File android_webview/native/aw_autofill_client.h (right): https://codereview.chromium.org/710453002/diff/260001/android_webview/native/... android_webview/native/aw_autofill_client.h:35: } On 2014/12/23 21:20:34, Peter Kasting wrote: > Seems like we shouldn't need this, the base class will have already declared it. Done. https://codereview.chromium.org/710453002/diff/260001/chrome/browser/autofill... File chrome/browser/autofill/autofill_cc_infobar_delegate_unittest.cc (right): https://codereview.chromium.org/710453002/diff/260001/chrome/browser/autofill... chrome/browser/autofill/autofill_cc_infobar_delegate_unittest.cc:59: }; On 2014/12/23 21:20:34, Peter Kasting wrote: > Nit: While here: DISALLOW_COPY_AND_ASSIGN() Done. https://codereview.chromium.org/710453002/diff/260001/chrome/browser/autofill... chrome/browser/autofill/autofill_cc_infobar_delegate_unittest.cc:75: // Associate the |ContentAutofillDriverFactory| to the |WebContents|. On 2014/12/23 21:20:34, Peter Kasting wrote: > Nit: Only use ||s for variable names, not type names. > > I'm not sure this comment adds much to the code anyway. I might just leave it > out. Done. https://codereview.chromium.org/710453002/diff/260001/chrome/browser/autofill... chrome/browser/autofill/autofill_cc_infobar_delegate_unittest.cc:94: On 2014/12/23 21:20:34, Peter Kasting wrote: > Nit: Blank here is unnecessary Done. https://codereview.chromium.org/710453002/diff/260001/chrome/browser/ui/autof... File chrome/browser/ui/autofill/chrome_autofill_client.h (right): https://codereview.chromium.org/710453002/diff/260001/chrome/browser/ui/autof... chrome/browser/ui/autofill/chrome_autofill_client.h:28: } On 2014/12/23 21:20:34, Peter Kasting wrote: > Again, I think we shouldn't need this forward decl. Done. https://codereview.chromium.org/710453002/diff/260001/components/autofill/con... File components/autofill/content/browser/content_autofill_driver.cc (right): https://codereview.chromium.org/710453002/diff/260001/components/autofill/con... components/autofill/content/browser/content_autofill_driver.cc:212: void ContentAutofillDriver::LinkClicked(const GURL& url, On 2014/12/23 21:20:34, Peter Kasting wrote: > The order of functions in the .cc file should match the declaration order in the > .h file. Done. https://codereview.chromium.org/710453002/diff/260001/components/autofill/con... File components/autofill/content/browser/content_autofill_driver.h (right): https://codereview.chromium.org/710453002/diff/260001/components/autofill/con... components/autofill/content/browser/content_autofill_driver.h:88: content::WebContents* web_contents(); On 2014/12/23 21:20:34, Peter Kasting wrote: > unix_hacker()-named functions should be simple inlined accessors, otherwise this > should be named GetWebContents(). Done. Renamed to GetWebContents(). https://codereview.chromium.org/710453002/diff/260001/components/autofill/cor... File components/autofill/core/browser/autofill_cc_infobar_delegate.h (right): https://codereview.chromium.org/710453002/diff/260001/components/autofill/cor... components/autofill/core/browser/autofill_cc_infobar_delegate.h:30: // to |autofill_driver|. On 2014/12/23 21:20:34, Peter Kasting wrote: > This comment isn't right; you don't add infobars to the autofill driver, you add > them to the infobar manager. Done. Rephrased comments to: Creates an autofill credit card infobar and delegate and add the infobar and associated |autofill_driver| to the |infobar_manager|. https://codereview.chromium.org/710453002/diff/260001/components/autofill/cor... components/autofill/core/browser/autofill_cc_infobar_delegate.h:66: base::WeakPtr<AutofillDriver> autofill_driver_; On 2014/12/23 21:20:34, Peter Kasting wrote: > If this lasts for the life of the tab, do we really need to hold it as a > WeakPtr? Infobars should be torn down as part of the tab destruction order, so > I would somewhat expect us to be able to hold this as a raw pointer. Did you > verify that's not possible? AutofillDriver are per render-frame within web contents. I'm little skeptical for cases where driver associated to sub-render frames (iFrmaes) & hence kept it as base::WeakPtr<> to gracefully handle any accidental access. For now I'm keeping this as is. https://codereview.chromium.org/710453002/diff/260001/components/autofill/cor... File components/autofill/core/browser/autofill_client.h (right): https://codereview.chromium.org/710453002/diff/260001/components/autofill/cor... components/autofill/core/browser/autofill_client.h:136: // Returns the |InfoBarManager| instance associated with the client. On 2014/12/23 21:20:34, Peter Kasting wrote: > Nit: No || Done. https://codereview.chromium.org/710453002/diff/260001/components/autofill/cor... components/autofill/core/browser/autofill_client.h:137: virtual infobars::InfoBarManager* GetInfoBarManager() = 0; On 2014/12/23 21:20:35, Peter Kasting wrote: > Nit: I'd put this up with the other "Gets the X associated with the client" > methods at the top of this list. (Also in subclasses) Done. https://codereview.chromium.org/710453002/diff/260001/components/autofill/cor... File components/autofill/core/browser/autofill_driver.h (right): https://codereview.chromium.org/710453002/diff/260001/components/autofill/cor... components/autofill/core/browser/autofill_driver.h:96: // infobar. On 2014/12/23 21:20:35, Peter Kasting wrote: > Nit: How about: > > Opens |url| with the supplied |disposition|. > > (This code doesn't really care in what circumstances clients might call it, so > it doesn't need to describe that.) Done. https://codereview.chromium.org/710453002/diff/260001/components/autofill/cor... File components/autofill/core/browser/autofill_manager.cc (right): https://codereview.chromium.org/710453002/diff/260001/components/autofill/cor... components/autofill/core/browser/autofill_manager.cc:675: AutofillCCInfoBarDelegate::Create( On 2014/12/23 21:20:35, Peter Kasting wrote: > It seems like the way we abstracted this entire call behind a client method > before was a better route. > > If this is abstracted behind a client call, we don't need to add the > GetInfoBarManager() method on the client, because the chrome-side code that > implements this can just do that directly to pass the value to > AutofillCCInfoBarDelegate::Create(). This also prevents any possible null-deref > in test code, where the implementation of GetInfoBarManager() returns null but > the infobar delegate doesn't null-check the provided pointer; instead, if this > is abstracted, the test implementation of that method can just do nothing. Done. Restored back ConfirmSaveCreditCard().
Patchset #15 (id:280001) has been deleted
(Have not re-reviewed) https://codereview.chromium.org/710453002/diff/260001/components/autofill/cor... File components/autofill/core/browser/autofill_cc_infobar_delegate.h (right): https://codereview.chromium.org/710453002/diff/260001/components/autofill/cor... components/autofill/core/browser/autofill_cc_infobar_delegate.h:66: base::WeakPtr<AutofillDriver> autofill_driver_; On 2014/12/24 11:16:57, Pritam Nikam wrote: > On 2014/12/23 21:20:34, Peter Kasting wrote: > > If this lasts for the life of the tab, do we really need to hold it as a > > WeakPtr? Infobars should be torn down as part of the tab destruction order, > so > > I would somewhat expect us to be able to hold this as a raw pointer. Did you > > verify that's not possible? > > AutofillDriver are per render-frame within web contents. I'm little skeptical > for cases where driver associated to sub-render frames (iFrmaes) & hence kept it > as base::WeakPtr<> to gracefully handle any accidental access. For now I'm > keeping this as is. I didn't fully understand that explanation, but it sounds as if you're not 100% sure you need this, you're more doing this "to be safe". If so, please find out for certain whether you need this, and if you do, document here in the code an example of when this can be destroyed. Otherwise change it to a raw pointer. Future people reading the code are going to assume you have to have this if you leave it in, and it will cause them to use WeakPtr in other similar classes as other infobars are componentized, and over time obscure the actual lifetime. Much like with null checks, we should only go down this road if we have to have it.
On 2014/12/24 23:23:25, Peter Kasting wrote: > (Have not re-reviewed) > > https://codereview.chromium.org/710453002/diff/260001/components/autofill/cor... > File components/autofill/core/browser/autofill_cc_infobar_delegate.h (right): > > https://codereview.chromium.org/710453002/diff/260001/components/autofill/cor... > components/autofill/core/browser/autofill_cc_infobar_delegate.h:66: > base::WeakPtr<AutofillDriver> autofill_driver_; > On 2014/12/24 11:16:57, Pritam Nikam wrote: > > On 2014/12/23 21:20:34, Peter Kasting wrote: > > > If this lasts for the life of the tab, do we really need to hold it as a > > > WeakPtr? Infobars should be torn down as part of the tab destruction order, > > so > > > I would somewhat expect us to be able to hold this as a raw pointer. Did > you > > > verify that's not possible? > > > > AutofillDriver are per render-frame within web contents. I'm little skeptical > > for cases where driver associated to sub-render frames (iFrmaes) & hence kept > it > > as base::WeakPtr<> to gracefully handle any accidental access. For now I'm > > keeping this as is. > > I didn't fully understand that explanation, but it sounds as if you're not 100% > sure you need this, you're more doing this "to be safe". > > If so, please find out for certain whether you need this, and if you do, > document here in the code an example of when this can be destroyed. Otherwise > change it to a raw pointer. > > Future people reading the code are going to assume you have to have this if you > leave it in, and it will cause them to use WeakPtr in other similar classes as > other infobars are componentized, and over time obscure the actual lifetime. > Much like with null checks, we should only go down this road if we have to have > it. Thanks Peter. I've tried elaborating it in comments, PTAL. Thanks!
https://codereview.chromium.org/710453002/diff/320001/components/autofill/con... File components/autofill/content/browser/content_autofill_driver.cc (right): https://codereview.chromium.org/710453002/diff/320001/components/autofill/con... components/autofill/content/browser/content_autofill_driver.cc:151: GetWebContents()->OpenURL(content::OpenURLParams( If this is the only use of GetWebContents(), then I'd simply inline its contents here (perhaps using a temporary variable) instead of making a new function for it. https://codereview.chromium.org/710453002/diff/320001/components/autofill/cor... File components/autofill/core/browser/autofill_cc_infobar_delegate.cc (right): https://codereview.chromium.org/710453002/diff/320001/components/autofill/cor... components/autofill/core/browser/autofill_cc_infobar_delegate.cc:103: GURL(autofill::kHelpURL), If this is the only call to LinkClicked(), I would remove this parameter and have LinkClicked() compute it instead, assuming it has visibility to this symbol. https://codereview.chromium.org/710453002/diff/320001/components/autofill/cor... File components/autofill/core/browser/autofill_cc_infobar_delegate.h (right): https://codereview.chromium.org/710453002/diff/320001/components/autofill/cor... components/autofill/core/browser/autofill_cc_infobar_delegate.h:29: // Creates an autofill credit card infobar and delegate and add the infobar The old comment was correct (except for using infobar_service where we now want infobar_manager). You don't add an autofill driver to the infobar manager, and your change of "adds" to "add" is a grammar error. https://codereview.chromium.org/710453002/diff/320001/components/autofill/cor... components/autofill/core/browser/autofill_cc_infobar_delegate.h:70: // LinkClinked() from infobar would be catastrophic. This comment is confusing, in part because of its grammar/mechanics errors, but also because I don't know if I understand what you're saying. Are you saying that it's possible to have an autofill infobar related to iframe content, and then have the iframe be destroyed, but the infobar not be removed? If so, that seems like a bug: why isn't the infobar being closed when the content it's related to is destroyed? Or is that not the scenario you're describing? And why wasn't this a problem in the old code, which unconditionally derefed WebContentsFromInfoBar()? It seems like that always assumed that there was a matching WebContents for the infobar, but now you're saying there isn't. Was the code before broken, or are you speaking in hypotheticals instead of about something you've actually reproduced? https://codereview.chromium.org/710453002/diff/320001/components/autofill/cor... File components/autofill/core/browser/autofill_client.h (right): https://codereview.chromium.org/710453002/diff/320001/components/autofill/cor... components/autofill/core/browser/autofill_client.h:85: // data. |autofill_driver| abstracts call to open url. |metric_logger| can be The added sentence here is meaningless: what call and what URL? Instead, this function's comment should say something about opening an infobar to prompt the user, and it probably doesn't even need to mention the driver.
Thanks Peter. PTAL at new patch set. Thanks! https://codereview.chromium.org/710453002/diff/320001/components/autofill/con... File components/autofill/content/browser/content_autofill_driver.cc (right): https://codereview.chromium.org/710453002/diff/320001/components/autofill/con... components/autofill/content/browser/content_autofill_driver.cc:151: GetWebContents()->OpenURL(content::OpenURLParams( On 2014/12/29 22:24:30, Peter Kasting wrote: > If this is the only use of GetWebContents(), then I'd simply inline its contents > here (perhaps using a temporary variable) instead of making a new function for > it. Done. https://codereview.chromium.org/710453002/diff/320001/components/autofill/cor... File components/autofill/core/browser/autofill_cc_infobar_delegate.cc (right): https://codereview.chromium.org/710453002/diff/320001/components/autofill/cor... components/autofill/core/browser/autofill_cc_infobar_delegate.cc:103: GURL(autofill::kHelpURL), On 2014/12/29 22:24:30, Peter Kasting wrote: > If this is the only call to LinkClicked(), I would remove this parameter and > have LinkClicked() compute it instead, assuming it has visibility to this > symbol. This is modified to address Ilya's comment earlier [A]: [A]. https://codereview.chromium.org/710453002/diff/60001/components/autofill/cont... https://codereview.chromium.org/710453002/diff/320001/components/autofill/cor... File components/autofill/core/browser/autofill_cc_infobar_delegate.h (right): https://codereview.chromium.org/710453002/diff/320001/components/autofill/cor... components/autofill/core/browser/autofill_cc_infobar_delegate.h:29: // Creates an autofill credit card infobar and delegate and add the infobar On 2014/12/29 22:24:30, Peter Kasting wrote: > The old comment was correct (except for using infobar_service where we now want > infobar_manager). You don't add an autofill driver to the infobar manager, and > your change of "adds" to "add" is a grammar error. Done. https://codereview.chromium.org/710453002/diff/320001/components/autofill/cor... components/autofill/core/browser/autofill_cc_infobar_delegate.h:70: // LinkClinked() from infobar would be catastrophic. On 2014/12/29 22:24:30, Peter Kasting wrote: > This comment is confusing, in part because of its grammar/mechanics errors, but > also because I don't know if I understand what you're saying. Are you saying > that it's possible to have an autofill infobar related to iframe content, and > then have the iframe be destroyed, but the infobar not be removed? If so, that > seems like a bug: why isn't the infobar being closed when the content it's > related to is destroyed? Or is that not the scenario you're describing? > > And why wasn't this a problem in the old code, which unconditionally derefed > WebContentsFromInfoBar()? It seems like that always assumed that there was a > matching WebContents for the infobar, but now you're saying there isn't. Was > the code before broken, or are you speaking in hypotheticals instead of about > something you've actually reproduced? Sorry for confusion! maybe I didn't phrase it correctly. I'd share my understanding about life expectancies of different objects in below excerpt: A. WebContents life expectancy -> as long as tab is open (with switch "--process-per-tab") B. Infobar's life expectancy -> WebContents C. AutofillClient's life expectancy -> WebContents (One AutofillClient per WebContents) D. AutofillDriverFactory's life expectancy -> WebContents (One factory per WebContents) E. WebContents can have multiple RenderFrame (i.e. iframes). AutofillDriverFactory takes care of creating one AutofillDriver per RenderFrame within that WebContents. F. i.e. AutofillDriver's (& AutofillManager's) life expectancy -> RenderFrame (One AutofillDriver per RenderFrame). Going by above understanding, Infobar lives longer compare to AutofillDriver (associated to RenderFrames). In other words; cases where RenderFrames associated to the page contents get destroy; autofill driver *does not* outlive the infobar. If we cache raw AutofillDriver pointers (associated to RenderFrames) in Infobar, there's a chance that link clicked on infobar would be unsafe. Please correct me if I'm missing something. https://codereview.chromium.org/710453002/diff/320001/components/autofill/cor... File components/autofill/core/browser/autofill_client.h (right): https://codereview.chromium.org/710453002/diff/320001/components/autofill/cor... components/autofill/core/browser/autofill_client.h:85: // data. |autofill_driver| abstracts call to open url. |metric_logger| can be On 2014/12/29 22:24:30, Peter Kasting wrote: > The added sentence here is meaningless: what call and what URL? Instead, this > function's comment should say something about opening an infobar to prompt the > user, and it probably doesn't even need to mention the driver. Done.
https://codereview.chromium.org/710453002/diff/320001/components/autofill/cor... File components/autofill/core/browser/autofill_cc_infobar_delegate.h (right): https://codereview.chromium.org/710453002/diff/320001/components/autofill/cor... components/autofill/core/browser/autofill_cc_infobar_delegate.h:70: // LinkClinked() from infobar would be catastrophic. On 2014/12/30 13:22:14, Pritam Nikam wrote: > On 2014/12/29 22:24:30, Peter Kasting wrote: > > This comment is confusing, in part because of its grammar/mechanics errors, > but > > also because I don't know if I understand what you're saying. Are you saying > > that it's possible to have an autofill infobar related to iframe content, and > > then have the iframe be destroyed, but the infobar not be removed? If so, > that > > seems like a bug: why isn't the infobar being closed when the content it's > > related to is destroyed? Or is that not the scenario you're describing? > > > > And why wasn't this a problem in the old code, which unconditionally derefed > > WebContentsFromInfoBar()? It seems like that always assumed that there was a > > matching WebContents for the infobar, but now you're saying there isn't. Was > > the code before broken, or are you speaking in hypotheticals instead of about > > something you've actually reproduced? > > Sorry for confusion! maybe I didn't phrase it correctly. > > I'd share my understanding about life expectancies of different objects in below > excerpt: > > A. WebContents life expectancy -> as long as tab is open (with switch > "--process-per-tab") > B. Infobar's life expectancy -> WebContents > C. AutofillClient's life expectancy -> WebContents (One AutofillClient per > WebContents) > D. AutofillDriverFactory's life expectancy -> WebContents (One factory per > WebContents) > E. WebContents can have multiple RenderFrame (i.e. iframes). > AutofillDriverFactory takes care of creating one AutofillDriver per RenderFrame > within that WebContents. > F. i.e. AutofillDriver's (& AutofillManager's) life expectancy -> RenderFrame > (One AutofillDriver per RenderFrame). > > Going by above understanding, Infobar lives longer compare to AutofillDriver > (associated to RenderFrames). In other words; cases where RenderFrames > associated to the page contents get destroy; autofill driver *does not* outlive > the infobar. > > If we cache raw AutofillDriver pointers (associated to RenderFrames) in Infobar, > there's a chance that link clicked on infobar would be unsafe. > > > Please correct me if I'm missing something. You keep speaking in abstract/hypothetical terms. I'm trying to stop talking about hypothetical possibilities, and start verifying whether we can really cause problems in actual usage. In other words, you need to verify, concretely, that you can actually have a problem here. If you can, the fix is probably not to use WeakPtrs, but either: (1) to make sure the infobar is removed if its associated page content is destroyed, or (2) to make AutofillDriver scoped to the WebContents, not the RenderFrame For example, crate a page with an inner frame that triggers the infobar, followed by the containing frame destroying the inner frame. Something like that.
On 2014/12/30 18:57:28, Peter Kasting wrote: > https://codereview.chromium.org/710453002/diff/320001/components/autofill/cor... > File components/autofill/core/browser/autofill_cc_infobar_delegate.h (right): > > https://codereview.chromium.org/710453002/diff/320001/components/autofill/cor... > components/autofill/core/browser/autofill_cc_infobar_delegate.h:70: // > LinkClinked() from infobar would be catastrophic. > On 2014/12/30 13:22:14, Pritam Nikam wrote: > > On 2014/12/29 22:24:30, Peter Kasting wrote: > > > This comment is confusing, in part because of its grammar/mechanics errors, > > but > > > also because I don't know if I understand what you're saying. Are you > saying > > > that it's possible to have an autofill infobar related to iframe content, > and > > > then have the iframe be destroyed, but the infobar not be removed? If so, > > that > > > seems like a bug: why isn't the infobar being closed when the content it's > > > related to is destroyed? Or is that not the scenario you're describing? > > > > > > And why wasn't this a problem in the old code, which unconditionally derefed > > > WebContentsFromInfoBar()? It seems like that always assumed that there was > a > > > matching WebContents for the infobar, but now you're saying there isn't. > Was > > > the code before broken, or are you speaking in hypotheticals instead of > about > > > something you've actually reproduced? > > > > Sorry for confusion! maybe I didn't phrase it correctly. > > > > I'd share my understanding about life expectancies of different objects in > below > > excerpt: > > > > A. WebContents life expectancy -> as long as tab is open (with switch > > "--process-per-tab") > > B. Infobar's life expectancy -> WebContents > > C. AutofillClient's life expectancy -> WebContents (One AutofillClient per > > WebContents) > > D. AutofillDriverFactory's life expectancy -> WebContents (One factory per > > WebContents) > > E. WebContents can have multiple RenderFrame (i.e. iframes). > > AutofillDriverFactory takes care of creating one AutofillDriver per > RenderFrame > > within that WebContents. > > F. i.e. AutofillDriver's (& AutofillManager's) life expectancy -> RenderFrame > > (One AutofillDriver per RenderFrame). > > > > Going by above understanding, Infobar lives longer compare to AutofillDriver > > (associated to RenderFrames). In other words; cases where RenderFrames > > associated to the page contents get destroy; autofill driver *does not* > outlive > > the infobar. > > > > If we cache raw AutofillDriver pointers (associated to RenderFrames) in > Infobar, > > there's a chance that link clicked on infobar would be unsafe. > > > > > > Please correct me if I'm missing something. > > You keep speaking in abstract/hypothetical terms. I'm trying to stop talking > about hypothetical possibilities, and start verifying whether we can really > cause problems in actual usage. > > In other words, you need to verify, concretely, that you can actually have a > problem here. If you can, the fix is probably not to use WeakPtrs, but either: > (1) to make sure the infobar is removed if its associated page content is > destroyed, or > (2) to make AutofillDriver scoped to the WebContents, not the RenderFrame > > For example, crate a page with an inner frame that triggers the infobar, > followed by the containing frame destroying the inner frame. Something like > that. Sorry Peter! I didn't share my findings with experiments I did earlier: Examples are as below: credit_card_test.html <!DOCTYPE html> <head><title>cc test</title></head> <body> <form id="testform" action="done.html" method="post"> <label>Name on Card: </label> <input type="text" name="cc_name"/> <p> <label for="card_number">Card Number: </label> <input type="text" name="cc_number"/> <p> <label for="expirationdate">Expiration Date: </label> <input type="text" name="cc_expiration_mm" maxlength="2" size="2" /> <input type="text" name="cc_expiration_yy" maxlength="2" size="2" /> <p> <input type="submit" value="Submit form"> </form> </body> done.html <!DOCTYPE html> <head> <title> Form submitted. </title> </head> <body> <label> Your Form Submitted! </label> </body> iframe_test.html <!DOCTYPE html> <head> <title>test</title> <style type="text/css"> .top { height:100px; width:200px; background-color:green; } </style> <script type="text/javascript"> function removeIFrame() { var frame = document.getElementById("target"); frame.parentNode.removeChild(frame); } </script> </head> <body> <div class="top" onclick="removeIFrame();"></div> <iframe id="target" src="credit_card_test.html"></iframe> <div class="top"></div> </body> A. Steps: 1. Open a credit_card_test.html page, fill in all credit card details and submit. 2. Chromium opens infobar and prompts users to save card details. 3. Open any other site in same tab or open chrome schema page (e.g. chrome://settings), infobar *does not* go away, now click on "Learn more" link from infobar. B. Steps: [Sample iframe case] 1. Open a iframe_test.html page, fill in all details cc details and submit. 2. Chromium opens infobar and prompts users to save card details. 3. Tap on *green* div, it will result in closing the iframe. 4. infobar *does not* go away, now click on "Learn more" link from infobar. Thanks!
Hi Peter, I've tried removing the infobar if its associated page content is destroyed, PTAL. Thanks!
Patchset #18 (id:360001) has been deleted
Patchset #18 (id:360001) has been deleted
On 2015/01/02 15:40:03, Pritam Nikam wrote: > I've tried removing the infobar if its associated page content is destroyed, Based on your descriptions I'm not sure that's actually right. Before this patch, the infobars would stick around, and clicking links on them would work properly, right? If the only reason we need the AutofillDriver is to have someone that can open links for us, why isn't the right answer to scope the driver to the WebContents instead of the frame? That would preserve the previous behavior, right? Honestly, I'm not even an autofill owner, I think Ilya should be weighing in here.
In terms of the lifetime discussion, I don't actually understand what guarantees are provided about when infobars are destroyed. I believe that the WebContents, any infobars associated with it, and the AutofillClient associated with it, are all destroyed at roughly the same time. I do not know what the specific guarantees are about the order. If there aren't any specific guarantees, then I'd prefer not to assume that there isn't a race condition just because we can't reproduce it via manual testing -- race conditions are very hard to repro via manual testing. https://codereview.chromium.org/710453002/diff/320001/components/autofill/cor... File components/autofill/core/browser/autofill_cc_infobar_delegate.cc (right): https://codereview.chromium.org/710453002/diff/320001/components/autofill/cor... components/autofill/core/browser/autofill_cc_infobar_delegate.cc:103: GURL(autofill::kHelpURL), On 2014/12/30 13:22:13, Pritam Nikam wrote: > On 2014/12/29 22:24:30, Peter Kasting wrote: > > If this is the only call to LinkClicked(), I would remove this parameter and > > have LinkClicked() compute it instead, assuming it has visibility to this > > symbol. > > This is modified to address Ilya's comment earlier [A]: > > [A]. > https://codereview.chromium.org/710453002/diff/60001/components/autofill/cont... > The goal is to share as much code as possible with iOS. I don't see any reason to duplicate the reference to the symbol in both iOS and content-based code. https://codereview.chromium.org/710453002/diff/380001/components/autofill/con... File components/autofill/content/browser/content_autofill_driver.cc (right): https://codereview.chromium.org/710453002/diff/380001/components/autofill/con... components/autofill/content/browser/content_autofill_driver.cc:155: } When you started working on this CL, there was one driver per WebContents, rather than one driver per frame. At this point, it might be easier to instead provide LinkClicked as a method on the client. https://codereview.chromium.org/710453002/diff/380001/components/autofill/cor... File components/autofill/core/browser/autofill_cc_infobar_delegate.cc (right): https://codereview.chromium.org/710453002/diff/380001/components/autofill/cor... components/autofill/core/browser/autofill_cc_infobar_delegate.cc:44: } This all seems needlessly complex. I think we should do one of the following three things: (1) Verify that the appropriate objects have guaranteed lifetimes that make null pointer dereferences impossible (even as a race condition). (2) Carefully document why no race condition is possible, even if it's not obvious just from lifetime considerations. (3) Just use a WeakPtr, which is much easier to reason about. https://codereview.chromium.org/710453002/diff/380001/components/autofill/cor... File components/autofill/core/browser/autofill_driver.h (right): https://codereview.chromium.org/710453002/diff/380001/components/autofill/cor... components/autofill/core/browser/autofill_driver.h:10: #include "base/memory/weak_ptr.h" nit: Looks like this is currently unused.
https://codereview.chromium.org/710453002/diff/380001/components/autofill/cor... File components/autofill/core/browser/autofill_cc_infobar_delegate.cc (right): https://codereview.chromium.org/710453002/diff/380001/components/autofill/cor... components/autofill/core/browser/autofill_cc_infobar_delegate.cc:44: } On 2015/01/06 02:54:59, Ilya Sherman wrote: > This all seems needlessly complex. I think we should do one of the following > three things: > > (1) Verify that the appropriate objects have guaranteed lifetimes that make null > pointer dereferences impossible (even as a race condition). > (2) Carefully document why no race condition is possible, even if it's not > obvious just from lifetime considerations. > (3) Just use a WeakPtr, which is much easier to reason about. (4) Provide LinkClicked as a method on the client and not need to change AutofillDriver's lifetime, associate it with an infobar delegate at all, etc. Or in some other way avoid this whole connection of driver to infobar. As Ilya says, it's horribly complex, and I'm opposed to the WeakPtr alternative that was previously being used for a variety of reasons.
Patchset #19 (id:400001) has been deleted
Thanks Ilya & Peter for inputs. AutofillClient outlives the lifetime of infobar. I've abstracting the LinkClicked() API through the AutofillClient, PTAL. Thanks! https://codereview.chromium.org/710453002/diff/380001/components/autofill/con... File components/autofill/content/browser/content_autofill_driver.cc (right): https://codereview.chromium.org/710453002/diff/380001/components/autofill/con... components/autofill/content/browser/content_autofill_driver.cc:155: } On 2015/01/06 02:54:59, Ilya Sherman wrote: > When you started working on this CL, there was one driver per WebContents, > rather than one driver per frame. At this point, it might be easier to instead > provide LinkClicked as a method on the client. Done. Abstracting the call to LinkClicked throuh the AutofillClient. https://codereview.chromium.org/710453002/diff/380001/components/autofill/cor... File components/autofill/core/browser/autofill_cc_infobar_delegate.cc (right): https://codereview.chromium.org/710453002/diff/380001/components/autofill/cor... components/autofill/core/browser/autofill_cc_infobar_delegate.cc:44: } On 2015/01/06 03:01:50, Peter Kasting wrote: > On 2015/01/06 02:54:59, Ilya Sherman wrote: > > This all seems needlessly complex. I think we should do one of the following > > three things: > > > > (1) Verify that the appropriate objects have guaranteed lifetimes that make > null > > pointer dereferences impossible (even as a race condition). > > (2) Carefully document why no race condition is possible, even if it's not > > obvious just from lifetime considerations. > > (3) Just use a WeakPtr, which is much easier to reason about. > > (4) Provide LinkClicked as a method on the client and not need to change > AutofillDriver's lifetime, associate it with an infobar delegate at all, etc. > > Or in some other way avoid this whole connection of driver to infobar. As Ilya > says, it's horribly complex, and I'm opposed to the WeakPtr alternative that was > previously being used for a variety of reasons. Done. https://codereview.chromium.org/710453002/diff/380001/components/autofill/cor... File components/autofill/core/browser/autofill_driver.h (right): https://codereview.chromium.org/710453002/diff/380001/components/autofill/cor... components/autofill/core/browser/autofill_driver.h:10: #include "base/memory/weak_ptr.h" On 2015/01/06 02:54:59, Ilya Sherman wrote: > nit: Looks like this is currently unused. Done.
It looks to me like both the ChromeAutofillClient and the InfobarService classes are registered as WebContentsUserData objects on the WebContents. Hence, their relative destruction order is undefined. What guarantees that the infobar will be destroyed before the client is; and what guarantees that it's safe for the client to call a WebContents method while the WebContents is potentially being destroyed? https://codereview.chromium.org/710453002/diff/420001/components/autofill/cor... File components/autofill/core/browser/autofill_cc_infobar_delegate.h (right): https://codereview.chromium.org/710453002/diff/420001/components/autofill/cor... components/autofill/core/browser/autofill_cc_infobar_delegate.h:42: #endif nit: Please leave a blank line after this one.
On 2015/01/06 23:20:17, Ilya Sherman wrote: > It looks to me like both the ChromeAutofillClient and the InfobarService classes > are registered as WebContentsUserData objects on the WebContents. Hence, their > relative destruction order is undefined. What guarantees that the infobar will > be destroyed before the client is; and what guarantees that it's safe for the > client to call a WebContents method while the WebContents is potentially being > destroyed? InfoBarService responds to WebContentsDestroyed() by destroying itself and all infobars connected to it. WebContentsDestroyed() is called before the end of ~WebContentsImpl(), whereas user data should (I believe) be destroyed during the superclass ~SupportsUserData(), so unless something deletes the autofill client early, I think we should be guaranteed that the autofill client outlives all infobars.
On 2015/01/06 23:44:02, Peter Kasting wrote: > On 2015/01/06 23:20:17, Ilya Sherman wrote: > > It looks to me like both the ChromeAutofillClient and the InfobarService > classes > > are registered as WebContentsUserData objects on the WebContents. Hence, > their > > relative destruction order is undefined. What guarantees that the infobar > will > > be destroyed before the client is; and what guarantees that it's safe for the > > client to call a WebContents method while the WebContents is potentially being > > destroyed? > > InfoBarService responds to WebContentsDestroyed() by destroying itself and all > infobars connected to it. WebContentsDestroyed() is called before the end of > ~WebContentsImpl(), whereas user data should (I believe) be destroyed during the > superclass ~SupportsUserData(), so unless something deletes the autofill client > early, I think we should be guaranteed that the autofill client outlives all > infobars. Okay, I think I'm convinced that the current destruction ordering is fine -- thanks for helping me to reason through that. Thanks also, Pritam, for bearing with the long discussion on this CL -- it's important to get these design details right. I have just a few final nits: https://codereview.chromium.org/710453002/diff/420001/components/autofill/cor... File components/autofill/core/browser/autofill_cc_infobar_delegate.h (right): https://codereview.chromium.org/710453002/diff/420001/components/autofill/cor... components/autofill/core/browser/autofill_cc_infobar_delegate.h:29: // Creates an autofill credit card infobar and delegate and add the infobar nit: "delegate" -> "client" https://codereview.chromium.org/710453002/diff/420001/components/autofill/cor... components/autofill/core/browser/autofill_cc_infobar_delegate.h:29: // Creates an autofill credit card infobar and delegate and add the infobar nit: "add" -> "adds" https://codereview.chromium.org/710453002/diff/420001/components/autofill/cor... components/autofill/core/browser/autofill_cc_infobar_delegate.h:30: // to |infobar_manager|. Please document the lifetime expectation, i.e. that the |autofill_client| must outlive the infobar. https://codereview.chromium.org/710453002/diff/420001/components/autofill/cor... components/autofill/core/browser/autofill_cc_infobar_delegate.h:64: AutofillClient* autofill_client_; nit: Please change the type to "AutofillClient* const"
Thanks Peter and Ilya. I've addressed nits, please glance over new patch set (#20). Thanks! https://codereview.chromium.org/710453002/diff/420001/components/autofill/cor... File components/autofill/core/browser/autofill_cc_infobar_delegate.h (right): https://codereview.chromium.org/710453002/diff/420001/components/autofill/cor... components/autofill/core/browser/autofill_cc_infobar_delegate.h:29: // Creates an autofill credit card infobar and delegate and add the infobar On 2015/01/07 01:58:54, Ilya Sherman wrote: > nit: "add" -> "adds" Done. https://codereview.chromium.org/710453002/diff/420001/components/autofill/cor... components/autofill/core/browser/autofill_cc_infobar_delegate.h:29: // Creates an autofill credit card infobar and delegate and add the infobar On 2015/01/07 01:58:54, Ilya Sherman wrote: > nit: "delegate" -> "client" Here "delegate" is an infobar delegate. https://codereview.chromium.org/710453002/diff/420001/components/autofill/cor... components/autofill/core/browser/autofill_cc_infobar_delegate.h:30: // to |infobar_manager|. On 2015/01/07 01:58:54, Ilya Sherman wrote: > Please document the lifetime expectation, i.e. that the |autofill_client| must > outlive the infobar. Done. I've rephrased it to: "Creates an autofill credit card infobar and a delegate and adds the infobar to |infobar_manager|. The |autofill_client| must outlive the infobar." https://codereview.chromium.org/710453002/diff/420001/components/autofill/cor... components/autofill/core/browser/autofill_cc_infobar_delegate.h:42: #endif On 2015/01/06 23:20:17, Ilya Sherman wrote: > nit: Please leave a blank line after this one. Done. https://codereview.chromium.org/710453002/diff/420001/components/autofill/cor... components/autofill/core/browser/autofill_cc_infobar_delegate.h:64: AutofillClient* autofill_client_; On 2015/01/07 01:58:54, Ilya Sherman wrote: > nit: Please change the type to "AutofillClient* const" Done.
LGTM https://codereview.chromium.org/710453002/diff/440001/android_webview/native/... File android_webview/native/aw_autofill_client.cc (right): https://codereview.chromium.org/710453002/diff/440001/android_webview/native/... android_webview/native/aw_autofill_client.cc:171: return false; Nit: Shouldn't this also NOTIMPLEMENTED(), as we expect that having no implementation here is OK only because control flow shouldn't reach here? https://codereview.chromium.org/710453002/diff/440001/components/autofill/cor... File components/autofill/core/browser/autofill_cc_infobar_delegate.h (right): https://codereview.chromium.org/710453002/diff/440001/components/autofill/cor... components/autofill/core/browser/autofill_cc_infobar_delegate.h:29: // Creates an autofill credit card infobar and a delegate and adds the infobar Nit: Don't add 'a' here https://codereview.chromium.org/710453002/diff/440001/components/autofill/cor... components/autofill/core/browser/autofill_cc_infobar_delegate.h:64: // the lifetime of |this| object. Nit: I would just say "Guaranteed to outlive us" for the second sentence.
LGTM as well. Thanks again for your patience in working on this issue.
Thanks a lot for driving this work to completion, Pritam!
isherman@chromium.org changed reviewers: + benm@chromium.org, oshima@chromium.org
+oshima@chromium.org for chrome/app/theme/theme_resources.grd +benm@chromium.org for android_webview/* +blundell@chromium.org for components/resources/*
//components/resources lgtm
c/a/theme lgtm
Thanks all. Added new patch for review, PTAL. Thanks! https://codereview.chromium.org/710453002/diff/440001/android_webview/native/... File android_webview/native/aw_autofill_client.cc (right): https://codereview.chromium.org/710453002/diff/440001/android_webview/native/... android_webview/native/aw_autofill_client.cc:171: return false; On 2015/01/07 20:26:29, Peter Kasting wrote: > Nit: Shouldn't this also NOTIMPLEMENTED(), as we expect that having no > implementation here is OK only because control flow shouldn't reach here? Done. https://codereview.chromium.org/710453002/diff/440001/components/autofill/cor... File components/autofill/core/browser/autofill_cc_infobar_delegate.h (right): https://codereview.chromium.org/710453002/diff/440001/components/autofill/cor... components/autofill/core/browser/autofill_cc_infobar_delegate.h:29: // Creates an autofill credit card infobar and a delegate and adds the infobar On 2015/01/07 20:26:29, Peter Kasting wrote: > Nit: Don't add 'a' here Done. https://codereview.chromium.org/710453002/diff/440001/components/autofill/cor... components/autofill/core/browser/autofill_cc_infobar_delegate.h:64: // the lifetime of |this| object. On 2015/01/07 20:26:29, Peter Kasting wrote: > Nit: I would just say "Guaranteed to outlive us" for the second sentence. Done.
On 2015/01/08 at 06:15:41, pritam.nikam wrote: > Thanks all. > > Added new patch for review, PTAL. > Thanks! > > https://codereview.chromium.org/710453002/diff/440001/android_webview/native/... > File android_webview/native/aw_autofill_client.cc (right): > > https://codereview.chromium.org/710453002/diff/440001/android_webview/native/... > android_webview/native/aw_autofill_client.cc:171: return false; > On 2015/01/07 20:26:29, Peter Kasting wrote: > > Nit: Shouldn't this also NOTIMPLEMENTED(), as we expect that having no > > implementation here is OK only because control flow shouldn't reach here? > > Done. > > https://codereview.chromium.org/710453002/diff/440001/components/autofill/cor... > File components/autofill/core/browser/autofill_cc_infobar_delegate.h (right): > > https://codereview.chromium.org/710453002/diff/440001/components/autofill/cor... > components/autofill/core/browser/autofill_cc_infobar_delegate.h:29: // Creates an autofill credit card infobar and a delegate and adds the infobar > On 2015/01/07 20:26:29, Peter Kasting wrote: > > Nit: Don't add 'a' here > > Done. > > https://codereview.chromium.org/710453002/diff/440001/components/autofill/cor... > components/autofill/core/browser/autofill_cc_infobar_delegate.h:64: // the lifetime of |this| object. > On 2015/01/07 20:26:29, Peter Kasting wrote: > > Nit: I would just say "Guaranteed to outlive us" for the second sentence. > > Done. Thank you for driving this to completion pritam!
Hi benm, PTAL @ "android_webview/*". Thanks!
On 2015/01/12 14:55:47, Pritam Nikam wrote: > Hi benm, > PTAL @ "android_webview/*". > > Thanks! android_webview lgtm.
The CQ bit was checked by pritam.nikam@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/710453002/460001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
isherman@chromium.org changed reviewers: + sky@chromium.org
+sky for OWNERS review of the new ui dependencies in components/autofill/core/browser/DEPS
LGTM
The CQ bit was checked by isherman@chromium.org
The CQ bit was unchecked by isherman@chromium.org
Pritam, FYI, you have compile failures on Android.
The CQ bit was checked by pritam.nikam@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/710453002/480001
On 2015/01/12 19:04:14, Ilya Sherman wrote: > Pritam, FYI, you have compile failures on Android. Thank you all. I've fixed the breakage and proceeded with "commit". Thanks!
Message was sent while issue was closed.
Committed patchset #22 (id:480001)
Message was sent while issue was closed.
Patchset 22 (id:??) landed as https://crrev.com/2cd65ab9548cd1c4bca4e0da1f8a59a531d82566 Cr-Commit-Position: refs/heads/master@{#311238} |
