Summary: | [Chromium] Suggestion popup is not closed when view is scrolled | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Roman <romange> | ||||||||||||||
Component: | New Bugs | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Minor | CC: | commit-queue, evan, fishd, jorlow, playmobil, romange, tkent | ||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | PC | ||||||||||||||||
OS: | Linux | ||||||||||||||||
Attachments: |
|
Description
Roman
2010-05-06 06:36:54 PDT
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 |