Bug 49291 - Add capability for displaying warnings to autofill popup
Summary: Add capability for displaying warnings to autofill popup
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-11-09 17:32 PST by Ilya Sherman
Modified: 2010-11-15 03:37 PST (History)
7 users (show)

See Also:


Attachments
Patch (10.57 KB, patch)
2010-11-09 17:39 PST, Ilya Sherman
no flags Details | Formatted Diff | Diff
Patch (17.92 KB, patch)
2010-11-10 03:07 PST, Ilya Sherman
no flags Details | Formatted Diff | Diff
Patch (11.53 KB, patch)
2010-11-12 03:20 PST, Ilya Sherman
no flags Details | Formatted Diff | Diff
Patch (11.96 KB, patch)
2010-11-12 15:48 PST, Ilya Sherman
no flags Details | Formatted Diff | Diff
Patch (14.68 KB, patch)
2010-11-14 19:38 PST, Ilya Sherman
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ilya Sherman 2010-11-09 17:32:02 PST
Add capability for displaying warnings to autofill popup
Comment 1 Ilya Sherman 2010-11-09 17:39:44 PST
Created attachment 73445 [details]
Patch
Comment 2 Darin Adler 2010-11-09 18:03:37 PST
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?
Comment 3 Ilya Sherman 2010-11-10 03:07:27 PST
Created attachment 73478 [details]
Patch
Comment 4 Ilya Sherman 2010-11-10 03:12:03 PST
(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.)
Comment 5 Jay Civelli 2010-11-11 15:10:11 PST
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?
Comment 6 Ilya Sherman 2010-11-11 15:34:54 PST
(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.
Comment 7 Ilya Sherman 2010-11-12 03:20:36 PST
Created attachment 73727 [details]
Patch
Comment 8 Ilya Sherman 2010-11-12 03:24:32 PST
(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.
Comment 9 Jay Civelli 2010-11-12 09:53:42 PST
LGTM
Comment 10 Ilya Sherman 2010-11-12 15:48:35 PST
Created attachment 73784 [details]
Patch

Actually make the warning display italicized...
Comment 11 Kent Tamura 2010-11-14 18:03:42 PST
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 12 Ilya Sherman 2010-11-14 19:38:02 PST
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.
Comment 13 Ilya Sherman 2010-11-14 19:38:53 PST
Created attachment 73871 [details]
Patch
Comment 14 Kent Tamura 2010-11-14 19:44:46 PST
Comment on attachment 73871 [details]
Patch

ok. good.
Comment 15 WebKit Commit Bot 2010-11-15 03:37:38 PST
Comment on attachment 73871 [details]
Patch

Clearing flags on attachment: 73871

Committed r72001: <http://trac.webkit.org/changeset/72001>
Comment 16 WebKit Commit Bot 2010-11-15 03:37:44 PST
All reviewed patches have been landed.  Closing bug.