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.
Created attachment 55231 [details] patch
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.
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.
Created attachment 55983 [details] patch More explanations were added to the Changlelog. The popup hiding was added to the slow scroll callback as well.
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.
Created attachment 56182 [details] patch
Comment on attachment 56182 [details] patch Informal Review: LGTM
adding some reviewers
I suspect this is still at the wrong level. See comment #3.
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.
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?
(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.
What needs to be done in order to proceed with this CL?
> > > + 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.
Created attachment 56579 [details] patch
(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.
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
Created attachment 56816 [details] Patch for landing
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).
Created attachment 56820 [details] Patch for landing
Comment on attachment 56820 [details] Patch for landing Clearing flags on attachment: 56820 Committed r60044: <http://trac.webkit.org/changeset/60044>
All reviewed patches have been landed. Closing bug.
(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
Rolled out by http://trac.webkit.org/changeset/119688. I have filed another bug for the original issue: Bug 88506