Summary: | [BlackBerry] Implement features for find-in-page | ||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Andy Chen <andchen> | ||||||||||||||||||||||
Component: | New Bugs | Assignee: | Andy Chen <andchen> | ||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||
Severity: | Normal | CC: | mifenton, tonikitoo, webkit.review.bot | ||||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||
Attachments: |
|
Description
Andy Chen
2012-02-28 11:56:01 PST
Created attachment 129306 [details]
patch
Attachment 129306 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/blackberry/Api/WebPage.cpp',..." exit_code: 1
Source/WebKit/blackberry/WebKitSupport/InPageSearchManager.h:25: Should have a space between // and comment [whitespace/comments] [4]
Total errors found: 1 in 10 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 129308 [details]
patch
Created attachment 129321 [details]
patch
All namespace usages of WebCore:: should be removed from the .cpp's that have using namespace WebCore. // TODO should be replaced by // FIXME: Created attachment 129334 [details]
patch
Comment on attachment 129334 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=129334&action=review > Source/WebKit/blackberry/Api/WebPage.cpp:5588 > +void WebPagePrivate::frameUnloaded(const WebCore::Frame* frame) Forgot to remove WebCore:: Created attachment 129335 [details]
patch
Comment on attachment 129335 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=129335&action=review Some questions, mainly: Maybe you could add a ActiveSearchData class to centralize m_activeSearch{String,MatchFrame,MatchCount} in a follow up? > Source/WebKit/blackberry/WebKitSupport/DOMSupport.cpp:406 > +Frame* incrementFrame(Frame* curr, bool forward, bool wrapFlag) I am not a fan of this name 'incrementFrame', but I do not have a better suggestion. > Source/WebKit/blackberry/WebKitSupport/InPageSearchManager.cpp:62 > + if (m_activeMatch && !m_activeMatch.get()->boundaryPointsValid()) you can call m_activeMatch-> directly without the .get(). > Source/WebKit/blackberry/WebKitSupport/InPageSearchManager.cpp:69 > + m_activeMatchCount = m_webPage->m_page->markAllMatchesForText(text, CaseInsensitive, true, TextMatchMarkerLimit); add a /* whatIsThisBoolFor */-like comment here. > Source/WebKit/blackberry/WebKitSupport/InPageSearchManager.cpp:75 > + setMarkerActive(m_activeMatch, false); Ditto > Source/WebKit/blackberry/WebKitSupport/InPageSearchManager.cpp:95 > + | StartInSelection; do you need the StartInSelection bit, if you just cleared any possible selection in the block above? > Source/WebKit/blackberry/WebKitSupport/InPageSearchManager.cpp:108 > + m_currentActiveMatchFrame = DOMSupport::incrementFrame(m_currentActiveMatchFrame, forward, true); > + if (findAndMarkText(text, m_activeMatch.get(), findOptions)) > + return true; > + } while (m_currentActiveMatchFrame && startFrame != m_currentActiveMatchFrame); I think it could be "better" here to actually pass the Frame* you want to search text at, instead of the setting a class member m_currentActiveMatchFrame, and use it from findAndMarkText. That is how I would do it, but if you feel strong about how it is now, that is fine with me. > Source/WebKit/blackberry/WebKitSupport/InPageSearchManager.cpp:125 > + && m_activeSearchString == text.substring(0, m_activeSearchString.length())) prefix only? or sufix or a substring in general? > Source/WebKit/blackberry/WebKitSupport/InPageSearchManager.cpp:147 > -void InPageSearchManager::setMarkerActive(WebCore::Range* range, bool active) > +void InPageSearchManager::setMarkerActive(PassRefPtr<Range> range, bool active) I think you want Range* here. You are not passing any ownership, right? > Source/WebKit/blackberry/WebKitSupport/InPageSearchManager.cpp:164 > + if (frame == m_webPage->mainFrame()) // Don't need to unmark and notify client because the page will be destroyed. if it is the mainframe, do we need to do any of the above anyways? (In reply to comment #9) > (From update of attachment 129335 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=129335&action=review > > Source/WebKit/blackberry/WebKitSupport/InPageSearchManager.cpp:95 > > + | StartInSelection; > > do you need the StartInSelection bit, if you just cleared any possible selection in the block above? Correct, it's only needed in the cases with a selection. No real harm in setting it though if the Range provided is empty. > > > Source/WebKit/blackberry/WebKitSupport/InPageSearchManager.cpp:108 > > + m_currentActiveMatchFrame = DOMSupport::incrementFrame(m_currentActiveMatchFrame, forward, true); > > + if (findAndMarkText(text, m_activeMatch.get(), findOptions)) > > + return true; > > + } while (m_currentActiveMatchFrame && startFrame != m_currentActiveMatchFrame); > > I think it could be "better" here to actually pass the Frame* you want to search text at, instead of the setting a class member m_currentActiveMatchFrame, and use it from findAndMarkText. > > That is how I would do it, but if you feel strong about how it is now, that is fine with me. > Would that allow the removal of m_currentActiveMatchFrame entirely? If so that is preferred. > > Source/WebKit/blackberry/WebKitSupport/InPageSearchManager.cpp:125 > > + && m_activeSearchString == text.substring(0, m_activeSearchString.length())) > > prefix only? or sufix or a substring in general? > Prefix only, this case is when adding an additional character to a non-matched string. No match is possible. > > Source/WebKit/blackberry/WebKitSupport/InPageSearchManager.cpp:164 > > + if (frame == m_webPage->mainFrame()) // Don't need to unmark and notify client because the page will be destroyed. > > if it is the mainframe, do we need to do any of the above anyways? Yes, we aren't unloading the instance so on page navigation we need to clear the cached objects. View in context: https://bugs.webkit.org/attachment.cgi?id=129335&action=review > Source/WebKit/blackberry/WebKitSupport/DOMSupport.cpp:407 > +{ Agreed, this was pulled from Page.cpp though so I'm in favor of keeping it but we should include a comment as to the source. > > > Source/WebKit/blackberry/WebKitSupport/InPageSearchManager.cpp:164 > > > + if (frame == m_webPage->mainFrame()) // Don't need to unmark and notify client because the page will be destroyed. > > > > if it is the mainframe, do we need to do any of the above anyways? > > Yes, we aren't unloading the instance so on page navigation we need to clear the cached objects. I mean, if the frame being unloaded is the main frame, it are in page shutdown process, because Page holds the same Frame reference cross page load. (In reply to comment #9) > (From update of attachment 129335 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=129335&action=review > > Some questions, mainly: > > Maybe you could add a ActiveSearchData class to centralize m_activeSearch{String,MatchFrame,MatchCount} in a follow up? > If we have a ActiveSearchData class for those info, there will be only one instance of it in InPageSearchManager. I don't see the point of adding it. (In reply to comment #12) > > Yes, we aren't unloading the instance so on page navigation we need to clear the cached objects. > > I mean, if the frame being unloaded is the main frame, it are in page shutdown process, because Page holds the same Frame reference cross page load. WebPage* is not destroyed when the main frame is unloaded, so this object lives on. (In reply to comment #10) > (In reply to comment #9) > > (From update of attachment 129335 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=129335&action=review > > > > > > Source/WebKit/blackberry/WebKitSupport/InPageSearchManager.cpp:108 > > > + m_currentActiveMatchFrame = DOMSupport::incrementFrame(m_currentActiveMatchFrame, forward, true); > > > + if (findAndMarkText(text, m_activeMatch.get(), findOptions)) > > > + return true; > > > + } while (m_currentActiveMatchFrame && startFrame != m_currentActiveMatchFrame); > > > > I think it could be "better" here to actually pass the Frame* you want to search text at, instead of the setting a class member m_currentActiveMatchFrame, and use it from findAndMarkText. > > > > That is how I would do it, but if you feel strong about how it is now, that is fine with me. > > > > Would that allow the removal of m_currentActiveMatchFrame entirely? If so that is preferred. > (In reply to comment #10) > (In reply to comment #9) > > (From update of attachment 129335 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=129335&action=review > > > > > Source/WebKit/blackberry/WebKitSupport/InPageSearchManager.cpp:108 > > > + m_currentActiveMatchFrame = DOMSupport::incrementFrame(m_currentActiveMatchFrame, forward, true); > > > + if (findAndMarkText(text, m_activeMatch.get(), findOptions)) > > > + return true; > > > + } while (m_currentActiveMatchFrame && startFrame != m_currentActiveMatchFrame); > > > > I think it could be "better" here to actually pass the Frame* you want to search text at, instead of the setting a class member m_currentActiveMatchFrame, and use it from findAndMarkText. > > > > That is how I would do it, but if you feel strong about how it is now, that is fine with me. > > > > Would that allow the removal of m_currentActiveMatchFrame entirely? If so that is preferred. > I don't think m_currentActiveMatchFrame could be removed entirely. For this function, it could be called by passing in the pointer. (In reply to comment #9) > (From update of attachment 129335 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=129335&action=review > > > Source/WebKit/blackberry/WebKitSupport/InPageSearchManager.cpp:147 > > -void InPageSearchManager::setMarkerActive(WebCore::Range* range, bool active) > > +void InPageSearchManager::setMarkerActive(PassRefPtr<Range> range, bool active) > > I think you want Range* here. You are not passing any ownership, right? > Yes, I just need Range* here but I want make ranger.get() to be inside of the function and if I use RefPtr, it does not pass check-webkit-style. Should I just use raw pointer instead? (In reply to comment #16) > (In reply to comment #9) > > (From update of attachment 129335 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=129335&action=review > > > > > Source/WebKit/blackberry/WebKitSupport/InPageSearchManager.cpp:147 > > > -void InPageSearchManager::setMarkerActive(WebCore::Range* range, bool active) > > > +void InPageSearchManager::setMarkerActive(PassRefPtr<Range> range, bool active) > > > > I think you want Range* here. You are not passing any ownership, right? > > > > Yes, I just need Range* here but I want make ranger.get() to be inside of the function and if I use RefPtr, it does not pass check-webkit-style. > > Should I just use raw pointer instead? I mean use raw pointer for m_activeMatch. It is RefPtr<Range> now. Created attachment 129506 [details]
patch
Comment on attachment 129506 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=129506&action=review > Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.cpp:794 > void FrameLoaderClientBlackBerry::dispatchWillClose() > { > - m_webPagePrivate->m_inputHandler->frameUnloaded(m_frame); > + m_webPagePrivate->frameUnloaded(m_frame); > } maybe you should also update FrameLoaderClientBB::detachedFromParent2()? > Source/WebKit/blackberry/WebKitSupport/DOMSupport.cpp:412 > +// This function is copied from WebCore/page/Page.cpp. > +Frame* incrementFrame(Frame* curr, bool forward, bool wrapFlag) > +{ > + return forward > + ? curr->tree()->traverseNextWithWrap(wrapFlag) > + : curr->tree()->traversePreviousWithWrap(wrapFlag); > +} I would rather make it local in PageFindManage till someone needs it. > Source/WebKit/blackberry/WebKitSupport/InPageSearchManager.h:25 > +#include <wtf/PassRefPtr.h> unneeded now? Comment on attachment 129506 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=129506&action=review > Source/WebKit/blackberry/WebKitSupport/InPageSearchManager.cpp:98 > + currentActiveMatchFrame = m_activeMatch ? m_activeMatch->ownerDocument()->frame() : m_webPage->focusedOrMainFrame(); Why not initialize the variable with this logic? Or make it. Frame* currentActiveMatchFrame = !selection.isNone() && m_activeMatch ? m_activeMatch->ownerDocument()->frame() : m_webPage->focusedOrMainFrame(); > Source/WebKit/blackberry/WebKitSupport/InPageSearchManager.cpp:100 > + Frame* startFrame = currentActiveMatchFrame; Move this above the Do loop. > Source/WebKit/blackberry/WebKitSupport/InPageSearchManager.cpp:124 > + && m_activeSearchString.length() Should we include a check to make sure that text.length() is greater than m_activeSearchString.length() to avoid the substring? Or us text.startsWith() > Source/WebKit/blackberry/WebKitSupport/InPageSearchManager.cpp:158 > + Frame* currentActiveMatchFrame = !m_activeMatch ? 0 : m_activeMatch->ownerDocument()->frame(); if (!m_activeMatch) return; Would be beneficial here. > Source/WebKit/blackberry/WebKitSupport/InPageSearchManager.cpp:159 > + if (currentActiveMatchFrame == frame || m_webPage->mainFrame() == frame) { Why is the mainframe special cased? If the mainFrame is unloaded so are the subframes. (In reply to comment #19) > (From update of attachment 129506 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=129506&action=review > > > Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.cpp:794 > > void FrameLoaderClientBlackBerry::dispatchWillClose() > > { > > - m_webPagePrivate->m_inputHandler->frameUnloaded(m_frame); > > + m_webPagePrivate->frameUnloaded(m_frame); > > } > > maybe you should also update FrameLoaderClientBB::detachedFromParent2()? Will do. > > > Source/WebKit/blackberry/WebKitSupport/DOMSupport.cpp:412 > > +// This function is copied from WebCore/page/Page.cpp. > > +Frame* incrementFrame(Frame* curr, bool forward, bool wrapFlag) > > +{ > > + return forward > > + ? curr->tree()->traverseNextWithWrap(wrapFlag) > > + : curr->tree()->traversePreviousWithWrap(wrapFlag); > > +} > > I would rather make it local in PageFindManage till someone needs it. It was there until Mike suggested to move over. > > > Source/WebKit/blackberry/WebKitSupport/InPageSearchManager.h:25 > > +#include <wtf/PassRefPtr.h> > > unneeded now? Will remove. (In reply to comment #20) > (From update of attachment 129506 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=129506&action=review > > > Source/WebKit/blackberry/WebKitSupport/InPageSearchManager.cpp:98 > > + currentActiveMatchFrame = m_activeMatch ? m_activeMatch->ownerDocument()->frame() : m_webPage->focusedOrMainFrame(); > > Why not initialize the variable with this logic? Or make it. > Frame* currentActiveMatchFrame = !selection.isNone() && m_activeMatch ? m_activeMatch->ownerDocument()->frame() : m_webPage->focusedOrMainFrame(); > I'll change the logic. > > Source/WebKit/blackberry/WebKitSupport/InPageSearchManager.cpp:100 > > + Frame* startFrame = currentActiveMatchFrame; > > Move this above the Do loop. Will do. > > > Source/WebKit/blackberry/WebKitSupport/InPageSearchManager.cpp:124 > > + && m_activeSearchString.length() > > Should we include a check to make sure that text.length() is greater than m_activeSearchString.length() to avoid the substring? Or us text.startsWith() > I'll check. > > Source/WebKit/blackberry/WebKitSupport/InPageSearchManager.cpp:158 > > + Frame* currentActiveMatchFrame = !m_activeMatch ? 0 : m_activeMatch->ownerDocument()->frame(); > > if (!m_activeMatch) > return; > > Would be beneficial here. > If m_activeMatch==0, we don't need to clear other values? > > Source/WebKit/blackberry/WebKitSupport/InPageSearchManager.cpp:159 > > + if (currentActiveMatchFrame == frame || m_webPage->mainFrame() == frame) { > > Why is the mainframe special cased? If the mainFrame is unloaded so are the subframes. In both cases we need to clear those values. Comment on attachment 129506 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=129506&action=review >>> Source/WebKit/blackberry/WebKitSupport/InPageSearchManager.cpp:158 >>> + Frame* currentActiveMatchFrame = !m_activeMatch ? 0 : m_activeMatch->ownerDocument()->frame(); >> >> if (!m_activeMatch) >> return; >> >> Would be beneficial here. > > If m_activeMatch==0, we don't need to clear other values? If currentActiveMatchFrame == 0 you don't go into the block. >>> Source/WebKit/blackberry/WebKitSupport/InPageSearchManager.cpp:159 >>> + if (currentActiveMatchFrame == frame || m_webPage->mainFrame() == frame) { >> >> Why is the mainframe special cased? If the mainFrame is unloaded so are the subframes. > > In both cases we need to clear those values. So it's really that if there is no activeMatch you need to reset m_activeSearchString? Created attachment 129528 [details]
patch
Attachment 129528 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/blackberry/Api/WebPage.cpp',..." exit_code: 1
Source/WebKit/blackberry/WebKitSupport/InPageSearchManager.cpp:157: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5]
Total errors found: 1 in 10 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 129534 [details]
patch
Created attachment 129538 [details]
patch
Comment on attachment 129538 [details]
patch
Mike, please cq+ if you do not have any further requests
Comment on attachment 129538 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=129538&action=review > Source/WebKit/blackberry/WebKitSupport/InPageSearchManager.cpp:103 > + if (findAndMarkText(text, m_activeMatch.get(), currentActiveMatchFrame, findOptions)) m_activeMatch is always null in the case, why not pass 0 explicitly? > Source/WebKit/blackberry/WebKitSupport/InPageSearchManager.cpp:162 > + Frame* currentActiveMatchFrame = !m_activeMatch ? 0 : m_activeMatch->ownerDocument()->frame(); Remove this check against !m_activeMatch, it was just checked with an early return. > Source/WebKit/blackberry/WebKitSupport/InPageSearchManager.cpp:163 > + if (currentActiveMatchFrame == frame || m_webPage->mainFrame() == frame) { The extra check inside the early return removed the need for m_webPage->mainFrame() == frame > Source/WebKit/blackberry/WebKitSupport/InPageSearchManager.h:47 > void setMarkerActive(WebCore::Range*, bool active); active parameter should not be included. > Source/WebKit/blackberry/WebKitSupport/InPageSearchManager.h:49 > + bool shouldSearchForText(const String& text); text parameter should not be included. Created attachment 129703 [details]
patch
Attachment 129703 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'LayoutTests/ChangeLog', u'La..." exit_code: 1
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitDefines.h"
Source/WebKit2/WebProcess/gtk/WebProcessMainGtk.cpp:71: Use 0 instead of NULL. [readability/null] [5]
Total errors found: 1 in 192 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 129703 [details] patch Clearing flags on attachment: 129703 Committed r109506: <http://trac.webkit.org/changeset/109506> All reviewed patches have been landed. Closing bug. |