Add capability for displaying warnings to autofill popup
Created attachment 73445 [details] Patch
Comment on attachment 73445 [details] Patch It seems strange that we are adding this feature to PopupMenuClient. Can this be restricted to the Chromium platform code by using some multiple inheritance? Why do we need this concept in the cross-platform sources at all?
Created attachment 73478 [details] Patch
(In reply to comment #2) > (From update of attachment 73445 [details]) > It seems strange that we are adding this feature to PopupMenuClient. Can this be restricted to the Chromium platform code by using some multiple inheritance? Why do we need this concept in the cross-platform sources at all? You're right, this should probably only touch Chromium code. Does the update version look more along the right lines? (Some parts of it were slapped together in the manner of "sleepily appease the compiler gods", so it'll almost certainly need to be cleaned up some; but the general trajectory should hopefully be better.)
I am not reviewer, but here are my comments: > WebCore/platform/chromium/PopupMenuChromium.cpp:1089 > + clearSelection(); 4 space indent. > WebCore/platform/chromium/PopupMenuClientChromium.h:31 > + virtual bool itemIsWarning(unsigned listIndex) const = 0; I think it would have been better to add this to PopupMenuClient (like we did previously for item labels). > WebKit/chromium/src/WebViewImpl.cpp:-1946 > - m_autoFillPopupClient->setSuggestions( Is that call not required anymore?
(In reply to comment #5) > I am not reviewer, but here are my comments: Thanks for taking a look =) > > WebCore/platform/chromium/PopupMenuClientChromium.h:31 > > + virtual bool itemIsWarning(unsigned listIndex) const = 0; > > I think it would have been better to add this to PopupMenuClient (like we did previously for item labels). For the sake of consistency, we should probably either add this to PopupMenuClient or move the labels out of there. I don't know the codebase nearly well enough, so I'll defer to others' wisdom here. > > WebKit/chromium/src/WebViewImpl.cpp:-1946 > > - m_autoFillPopupClient->setSuggestions( > > Is that call not required anymore? It's called from within m_autoFillPopupClient->initialize(), so I think this call was redundant.
Created attachment 73727 [details] Patch
(In reply to comment #6) > (In reply to comment #5) > > > WebCore/platform/chromium/PopupMenuClientChromium.h:31 > > > + virtual bool itemIsWarning(unsigned listIndex) const = 0; > > > > I think it would have been better to add this to PopupMenuClient (like we did previously for item labels). > > For the sake of consistency, we should probably either add this to PopupMenuClient or move the labels out of there. I don't know the codebase nearly well enough, so I'll defer to others' wisdom here. The latest patch sidesteps this issue by moving all the code in question down into AutoFillPopupMenuClient.cpp. The original question is likely to become relevant again in the near future, though, as we might want to distinguish warnings by slightly more than just tweaking the font.
LGTM
Created attachment 73784 [details] Patch Actually make the warning display italicized...
Comment on attachment 73784 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=73784&action=review > WebCore/ChangeLog:3 > 2010-11-12 Ilya Sherman <isherman@chromium.org> > > + Reviewed by NOBODY (OOPS!). The ChangeLog diff should start at the top of ChangeLog. > WebCore/ChangeLog:12 > + * platform/chromium/PopupMenuChromium.cpp: > + (WebCore::PopupListBox::selectIndex): > + * platform/chromium/PopupMenuChromium.h: The list lacks PopupListBox::getRowFont(). We had better write what we change for each of files/functions as possible. > WebKit/chromium/ChangeLog:3 > 2010-11-12 Ilya Sherman <isherman@chromium.org> > > + Reviewed by NOBODY (OOPS!). The ChangeLog diff doesn't start at the top of ChangeLog. > WebKit/chromium/src/AutoFillPopupMenuClient.cpp:254 > + return m_uniqueIDs[index] == -1; The rule "uniqueID ==-1 -> warning" looks implicit. Can we introduce a symbol for this -1? I also found WebViewClient.h has no explanation of uniqueID.
Comment on attachment 73784 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=73784&action=review Thanks as always for the detailed feedback =) >> WebCore/ChangeLog:3 >> + Reviewed by NOBODY (OOPS!). > > The ChangeLog diff should start at the top of ChangeLog. It's the diff tool's fault! =) But, it should be fixed now, with some other changes interspersed between my two. >> WebKit/chromium/src/AutoFillPopupMenuClient.cpp:254 >> + return m_uniqueIDs[index] == -1; > > The rule "uniqueID ==-1 -> warning" looks implicit. Can we introduce a symbol for this -1? > > I also found WebViewClient.h has no explanation of uniqueID. Oops, this should actually be uniqueID < 0 -- updated the code to match. Also added comments to WebView.h and WebViewClient.h to explain the role of the uniqueID.
Created attachment 73871 [details] Patch
Comment on attachment 73871 [details] Patch ok. good.
Comment on attachment 73871 [details] Patch Clearing flags on attachment: 73871 Committed r72001: <http://trac.webkit.org/changeset/72001>
All reviewed patches have been landed. Closing bug.