Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
74563003:
Removed some GetActiveEntry references in autofill (Closed)
|
Created:
7 years, 1 month ago by jww Modified:
7 years, 1 month ago CC:
chromium-reviews, benquan, Dane Wallinga, dyu1, estade+watch_chromium.org, Ilya Sherman, rouslan+autofillwatch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionRemoved some GetActiveEntry references in autofill.
See also LGTMs in https://codereview.chromium.org/66993003/.
BUG=273710
TBR=creis@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=236291
Patch Set 1 #
Total comments: 2
Patch Set 2 : Fixed merge conflict #
Total comments: 4
Patch Set 3 : Fixed errors #
Total comments: 6
Patch Set 4 : Improved comment in Show() #
Total comments: 4
Patch Set 5 : Comment nits #
Messages
Total messages: 18 (0 generated)
No LGTM from a valid reviewer yet. Only full committers are accepted. Even if an LGTM may have been provided, it was from a non-committer or a lowly provisional committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
Hmm, not LGTM due to the merge conflict. You'll need to either add or TBR owners on each of these CLs as well. https://codereview.chromium.org/74563003/diff/1/chrome/browser/ui/autofill/au... File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (left): https://codereview.chromium.org/74563003/diff/1/chrome/browser/ui/autofill/au... chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:653: if (account_chooser_model_.WalletIsSelected()) Is this a merge conflict? I don't think you want to change this. https://codereview.chromium.org/74563003/diff/1/chrome/browser/ui/autofill/au... File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right): https://codereview.chromium.org/74563003/diff/1/chrome/browser/ui/autofill/au... chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:663: } else { Another merge conflict?
On 2013/11/16 19:13:16, creis wrote: > Hmm, not LGTM due to the merge conflict. You'll need to either add or TBR > owners on each of these CLs as well. > > https://codereview.chromium.org/74563003/diff/1/chrome/browser/ui/autofill/au... > File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (left): > > https://codereview.chromium.org/74563003/diff/1/chrome/browser/ui/autofill/au... > chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:653: if > (account_chooser_model_.WalletIsSelected()) > Is this a merge conflict? I don't think you want to change this. > > https://codereview.chromium.org/74563003/diff/1/chrome/browser/ui/autofill/au... > File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right): > > https://codereview.chromium.org/74563003/diff/1/chrome/browser/ui/autofill/au... > chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:663: } else { > Another merge conflict? Yikes! I certainly screwed that up. I'll upload a new CL shortly.
On 2013/11/19 21:22:24, jww wrote: > On 2013/11/16 19:13:16, creis wrote: > > Hmm, not LGTM due to the merge conflict. You'll need to either add or TBR > > owners on each of these CLs as well. > > > > > https://codereview.chromium.org/74563003/diff/1/chrome/browser/ui/autofill/au... > > File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (left): > > > > > https://codereview.chromium.org/74563003/diff/1/chrome/browser/ui/autofill/au... > > chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:653: if > > (account_chooser_model_.WalletIsSelected()) > > Is this a merge conflict? I don't think you want to change this. > > > > > https://codereview.chromium.org/74563003/diff/1/chrome/browser/ui/autofill/au... > > File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right): > > > > > https://codereview.chromium.org/74563003/diff/1/chrome/browser/ui/autofill/au... > > chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:663: } else { > > Another merge conflict? > > Yikes! I certainly screwed that up. I'll upload a new CL shortly. @creis, can you take a look at the new patchset? Thanks!
Hmm, still not right. https://codereview.chromium.org/74563003/diff/90001/chrome/browser/ui/android... File chrome/browser/ui/android/autofill/autofill_dialog_controller_android.cc (right): https://codereview.chromium.org/74563003/diff/90001/chrome/browser/ui/android... chrome/browser/ui/android/autofill/autofill_dialog_controller_android.cc:197: current_url.GetContent() == source_url_.GetOrigin(); Why is this GetContent()? It should be GetOrigin(). Compare to: https://codereview.chromium.org/66993003/diff/590001/chrome/browser/ui/androi... https://codereview.chromium.org/74563003/diff/90001/chrome/browser/ui/autofil... File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right): https://codereview.chromium.org/74563003/diff/90001/chrome/browser/ui/autofil... chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:580: web_contents()->GetController().GetActiveEntry(); Your previous patch deleted this line, since it's no longer used. https://codereview.chromium.org/66993003/diff/590001/chrome/browser/ui/autofi...
Arg, super embarrassing. Sorry about that. I think this patch fixes everything... https://codereview.chromium.org/74563003/diff/90001/chrome/browser/ui/android... File chrome/browser/ui/android/autofill/autofill_dialog_controller_android.cc (right): https://codereview.chromium.org/74563003/diff/90001/chrome/browser/ui/android... chrome/browser/ui/android/autofill/autofill_dialog_controller_android.cc:197: current_url.GetContent() == source_url_.GetOrigin(); On 2013/11/19 21:49:15, creis wrote: > Why is this GetContent()? It should be GetOrigin(). > > Compare to: > https://codereview.chromium.org/66993003/diff/590001/chrome/browser/ui/androi... Done. https://codereview.chromium.org/74563003/diff/90001/chrome/browser/ui/autofil... File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right): https://codereview.chromium.org/74563003/diff/90001/chrome/browser/ui/autofil... chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:580: web_contents()->GetController().GetActiveEntry(); On 2013/11/19 21:49:15, creis wrote: > Your previous patch deleted this line, since it's no longer used. > > https://codereview.chromium.org/66993003/diff/590001/chrome/browser/ui/autofi... Done.
Arg, super embarrassing. Sorry about that. I think this patch fixes everything...
No worries. LGTM!
On 2013/11/19 22:23:09, creis wrote: > No worries. LGTM! Phew, finally :-) Thanks a lot!=
https://codereview.chromium.org/74563003/diff/150001/chrome/browser/ui/autofi... File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right): https://codereview.chromium.org/74563003/diff/150001/chrome/browser/ui/autofi... chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:581: // the last committed URL for access checks. Hmm, I don't really see the value of this comment. To me, the comment actually obfuscates the fact that |invoked_from_same_origin_| is a security consideration, used to warn users when their data is requested from a cross-origin frame on the page, rather than from the top-level frame or a frame on the same domain as the top-level frame.
https://codereview.chromium.org/74563003/diff/150001/chrome/browser/ui/autofi... File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right): https://codereview.chromium.org/74563003/diff/150001/chrome/browser/ui/autofi... chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:579: // Autocomplete is show in response to a message from the renderer. As such, nit: shown https://codereview.chromium.org/74563003/diff/150001/chrome/browser/ui/autofi... chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:581: // the last committed URL for access checks. On 2013/11/19 22:56:24, Ilya Sherman wrote: > Hmm, I don't really see the value of this comment. It's apparently non-obvious, because there's a bug there now. GetActiveEntry would return the pending entry if there were one, which is incorrect. > To me, the comment actually > obfuscates the fact that |invoked_from_same_origin_| is a security > consideration, used to warn users when their data is requested from a > cross-origin frame on the page, rather than from the top-level frame or a frame > on the same domain as the top-level frame. Perhaps it's worth including that in the comment as well (first), and then explaining why LastCommitted is the correct thing to check against?
https://codereview.chromium.org/74563003/diff/150001/chrome/browser/ui/autofi... File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right): https://codereview.chromium.org/74563003/diff/150001/chrome/browser/ui/autofi... chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:581: // the last committed URL for access checks. On 2013/11/19 23:10:56, creis wrote: > On 2013/11/19 22:56:24, Ilya Sherman wrote: > > Hmm, I don't really see the value of this comment. > > It's apparently non-obvious, because there's a bug there now. GetActiveEntry > would return the pending entry if there were one, which is incorrect. Fair point, though I think that has more to do with the semantics of GetActiveEntry() being unclear (to me, at least). > > To me, the comment actually > > obfuscates the fact that |invoked_from_same_origin_| is a security > > consideration, used to warn users when their data is requested from a > > cross-origin frame on the page, rather than from the top-level frame or a > frame > > on the same domain as the top-level frame. > > Perhaps it's worth including that in the comment as well (first), and then > explaining why LastCommitted is the correct thing to check against? If I'm understanding you correctly, you're saying that the goal of this comment is more or less to explain why LastCommitted is more appropriate than ActiveEntry. If so, yes, I think that's fair; but then I'd urge Joel to explicitly call out GetActiveEntry() in the comment. Otherwise, without the context of the code that's changing in this CL, it's not obvious (again, to me) that that's what's being compared to. (Even with the context of the code that's changing in this CL, I didn't realize this was a semantic change. I thought it was a purely refactoring change to introduce a clearer method name or tighten an API or something like that.)
https://codereview.chromium.org/74563003/diff/150001/chrome/browser/ui/autofi... File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right): https://codereview.chromium.org/74563003/diff/150001/chrome/browser/ui/autofi... chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:579: // Autocomplete is show in response to a message from the renderer. As such, On 2013/11/19 23:10:56, creis wrote: > nit: shown Done. https://codereview.chromium.org/74563003/diff/150001/chrome/browser/ui/autofi... chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:581: // the last committed URL for access checks. On 2013/11/19 23:10:56, creis wrote: > On 2013/11/19 22:56:24, Ilya Sherman wrote: > > Hmm, I don't really see the value of this comment. > > It's apparently non-obvious, because there's a bug there now. GetActiveEntry > would return the pending entry if there were one, which is incorrect. > > > To me, the comment actually > > obfuscates the fact that |invoked_from_same_origin_| is a security > > consideration, used to warn users when their data is requested from a > > cross-origin frame on the page, rather than from the top-level frame or a > frame > > on the same domain as the top-level frame. > > Perhaps it's worth including that in the comment as well (first), and then > explaining why LastCommitted is the correct thing to check against? I've uploaded a new comment. @isherman, can you let me know if it is sufficiently clear or if I can improve it anymore?
LGTM % nits, thanks. https://codereview.chromium.org/74563003/diff/250001/chrome/browser/ui/autofi... File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right): https://codereview.chromium.org/74563003/diff/250001/chrome/browser/ui/autofi... chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:579: // Autocomplete is shown in response to a message from the renderer and as nit: "Autocomplete" -> "The Autofill dialog" https://codereview.chromium.org/74563003/diff/250001/chrome/browser/ui/autofi... chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:582: // be a security bug. Therefore, we must use the last committed URL for the nit: I'd recommend omitting "we must".
https://codereview.chromium.org/74563003/diff/250001/chrome/browser/ui/autofi... File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right): https://codereview.chromium.org/74563003/diff/250001/chrome/browser/ui/autofi... chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:579: // Autocomplete is shown in response to a message from the renderer and as On 2013/11/19 23:43:13, Ilya Sherman wrote: > nit: "Autocomplete" -> "The Autofill dialog" Done. https://codereview.chromium.org/74563003/diff/250001/chrome/browser/ui/autofi... chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:582: // be a security bug. Therefore, we must use the last committed URL for the On 2013/11/19 23:43:13, Ilya Sherman wrote: > nit: I'd recommend omitting "we must". Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jww@chromium.org/74563003/300001
Message was sent while issue was closed.
Change committed as 236291 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
