Bug 38640 – [Chromium] Suggestion popup is not closed when view is scrolled

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Roman]

When suggestion popup is opened and user scrolls the view either by mouse or using the scrollbar, the popup stays in the same position relative to the screen.
Expected behavior: popup should close.

[Roman]

Created attachment 55231 [details]
patch

[Evan Martin]

Needs a comment in the ChangeLog about testing, whether it's possible or why not.

Might be good to note in the ChangeLog why this already works properly in Windows/Mac.

Might be good to make it clearer that you're talking about suggestion popup.

[Darin Fisher (:fishd, Google)]

Comment on attachment 55231 [details]
patch

> Index: src/ChromeClientImpl.cpp
> ===================================================================
> --- src/ChromeClientImpl.cpp	(revision 58877)
> +++ src/ChromeClientImpl.cpp	(working copy)
> @@ -507,6 +507,7 @@ void ChromeClientImpl::scroll(

scroll is only called when we are doing accelerated scrolling.  if the
page contains a fixed position element, then invalidateRect will be used
to repaint the entire view.  so, this patch only solves part of the problem.

i think you should really hook into the same thing that triggers the
'scroll' event being dispatched to the page.

[Roman]

Created attachment 55983 [details]
patch

More explanations were added to the Changlelog.
The popup hiding was added to the slow scroll callback as well.

[Jeremy Moskovich]

Comment on attachment 55983 [details]
patch

Informal review:

Changelogs:
 +          
I think your editor is stripping trailing whitespaces from files, I don't think you want to do that for the changelog, the diff should only contain your additions.

if (m_webView->client()) {
WebKit coding style is no curly braces for one line ifs.

void ChromeClientImpl::scroll(
I'd move the call to hidePopups() to before the dx,dy variable definitions.

[Roman]

Created attachment 56182 [details]
patch

[Jeremy Moskovich]

Comment on attachment 56182 [details]
patch

Informal Review:

LGTM

[Evan Martin]

adding some reviewers

[Evan Martin]

I suspect this is still at the wrong level.  See comment #3.

[Roman]

I talked to Darin via chat and after inspecting the code he approved my latest change. But I am open for suggestions. 

Another option would be is to expose a special interface function in ChromeClientImpl just for hiding popups but it seems an overkill for me.

[Darin Fisher (:fishd, Google)]

Comment on attachment 56182 [details]
patch

> Index: src/ChromeClientImpl.cpp
...
>  void ChromeClientImpl::invalidateContentsForSlowScroll(const IntRect& updateRect, bool immediate)
>  {
> +    if (m_webView->client())
> +        m_webView->hidePopups();

why do you need to null check client before calling hidePopups?

[Roman]

(In reply to comment #11)
> (From update of attachment 56182 [details])
> > Index: src/ChromeClientImpl.cpp
> ...
> >  void ChromeClientImpl::invalidateContentsForSlowScroll(const IntRect& updateRect, bool immediate)
> >  {
> > +    if (m_webView->client())
> > +        m_webView->hidePopups();
> 
> why do you need to null check client before calling hidePopups?

I am being consistent with the code around. The scroll() function null checks the client so I assumed I need to do this as well.

[Roman]

What needs to be done in order to proceed with this CL?

[Darin Fisher (:fishd, Google)]

> > > +    if (m_webView->client())
> > > +        m_webView->hidePopups();
> > 
> > why do you need to null check client before calling hidePopups?
> 
> I am being consistent with the code around. The scroll() function null checks the client so I assumed I need to do this as well.

The null checks for m_webView->client() exist to avoid calling methods on a null m_webView->client().  It is not necessary in the case above since you are calling a method on m_webView.

[Roman]

Created attachment 56579 [details]
patch

[Roman]

(In reply to comment #14)
> > > > +    if (m_webView->client())
> > > > +        m_webView->hidePopups();
> > > 
> > > why do you need to null check client before calling hidePopups?
> > 
> > I am being consistent with the code around. The scroll() function null checks the client so I assumed I need to do this as well.
> 
> The null checks for m_webView->client() exist to avoid calling methods on a null m_webView->client().  It is not necessary in the case above since you are calling a method on m_webView.

Oops, I was under impression that I am using client() during the call. The fixed patch is uploaded.

[WebKit Commit Bot]

Comment on attachment 56579 [details]
patch

Rejecting patch 56579 from commit-queue.

Failed to run "[u'/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Darin Fisher', u'--force']" exit_code: 1
Last 500 characters of output:
file(s).
patching file ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
can't find file to patch at input line 5
Perhaps you used the wrong -p or --strip option?
The text leading up to this was:
--------------------------
|Index: src/ChromeClientImpl.cpp
|===================================================================
|--- src/ChromeClientImpl.cpp	(revision 59826)
|+++ src/ChromeClientImpl.cpp	(working copy)
--------------------------
No file to patch.  Skipping patch.
2 out of 2 hunks ignored

Full output: http://webkit-commit-queue.appspot.com/results/2272442

[Jeremy Moskovich]

Created attachment 56816 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 56816 [details]
Patch for landing

Rejecting patch 56816 from commit-queue.

Unexpected failure when landing patch!  Please file a bug against webkit-patch.
Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', 'land-attachment', '--force-clean', '--build', '--non-interactive', '--ignore-builders', '--build-style=both', '--quiet', 56816, '--test', '--parent-command=commit-queue', '--no-update']" exit_code: 1
Last 500 characters of output:
ion=edit
Fetching: https://bugs.webkit.org/show_bug.cgi?id=38640&ctype=xml
Processing 1 patch from 1 bug.
Cleaning working directory
Processing patch 56816 from bug 38640.
NOBODY (OOPS!) found in /Users/eseidel/Projects/CommitQueue/WebKit/chromium/ChangeLog does not appear to be a valid reviewer according to committers.py.
ERROR: /Users/eseidel/Projects/CommitQueue/WebKit/chromium/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

[Jeremy Moskovich]

Created attachment 56820 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 56820 [details]
Patch for landing

Clearing flags on attachment: 56820

Committed r60044: <http://trac.webkit.org/changeset/60044>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Kent Tamura]

(In reply to comment #21)
> (From update of attachment 56820 [details])
> Clearing flags on attachment: 56820
> 
> Committed r60044: <http://trac.webkit.org/changeset/60044>

I'll roll this out because of http://code.google.com/p/chromium/issues/detail?id=114922

[Kent Tamura]

Rolled out by http://trac.webkit.org/changeset/119688.
I have filed another bug for the original issue: Bug 88506