Bug 49291 – Add capability for displaying warnings to autofill popup

• Component: New Bugs
• Status: RESOLVED FIXED

[Ilya Sherman]

Add capability for displaying warnings to autofill popup

[Ilya Sherman]

Created attachment 73445 [details]
Patch

[Darin Adler]

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?

[Ilya Sherman]

Created attachment 73478 [details]
Patch

[Ilya Sherman]

(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.)

[Jay Civelli]

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?

[Ilya Sherman]

(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.

[Ilya Sherman]

Created attachment 73727 [details]
Patch

[Ilya Sherman]

(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.

[Jay Civelli]

LGTM

[Ilya Sherman]

Created attachment 73784 [details]
Patch

Actually make the warning display italicized...

[Kent Tamura]

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  <[e-mail]>
>  
> +        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  <[e-mail]>
>  
> +        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.

[Ilya Sherman]

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.

[Ilya Sherman]

Created attachment 73871 [details]
Patch

[Kent Tamura]

Comment on attachment 73871 [details]
Patch

ok. good.

[WebKit Commit Bot]

Comment on attachment 73871 [details]
Patch

Clearing flags on attachment: 73871

Committed r72001: <http://trac.webkit.org/changeset/72001>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.