Created attachment 94963 [details] proposed fix

Created attachment 94964 [details] proposed fix

Comment on attachment 94964 [details] proposed fix View in context: https://bugs.webkit.org/attachment.cgi?id=94964&action=review Disclaim: not a reviewer. :) > Tools/QtTestBrowser/launcherwindow.cpp:1032 > + page()->findText("", QFlag(8)); Is there an enum for 8? > Tools/QtTestBrowser/launcherwindow.cpp:1045 > + if (mode == BackWard) if -> else if > Tools/QtTestBrowser/launcherwindow.cpp:1047 > + if (mode == CaseSensitively) anyway, they should not be mutually exclusive. see further comments below. > Tools/QtTestBrowser/launcherwindow.cpp:1051 > + if (mode == HighLightAll) HighLightAll should not be an exclusive value. It can be "OR"-ed to other values. > Tools/QtTestBrowser/launcherwindow.h:252 > + WarpsAround, typo > Tools/QtTestBrowser/launcherwindow.h:254 > + }; It doesn't seem to be right to use enums for these options (casesensitive, wrap, highlight) since they are not mutually exclusive. You should use separate flags or bit-masks.

Comment on attachment 94964 [details] proposed fix View in context: https://bugs.webkit.org/attachment.cgi?id=94964&action=review LGTM, just few comments. > Tools/QtTestBrowser/launcherwindow.h:195 > + void find(int mode); This should be FindMode instead of int and you could set the default value here to Normal. > Tools/QtTestBrowser/launcherwindow.h:248 > + int m_findFlag; This could be QWebPage::FindFlag instead of int. > Tools/QtTestBrowser/launcherwindow.h:254 > + enum FindMode { Normal, > + BackWard, > + CaseSensitively, > + WarpsAround, > + HighLightAll > + }; You could use QWebPage::FindFlag, and have a constant for Normal instead of an extra FindMode enum here.

Comment on attachment 94964 [details] proposed fix r- because of the comments and it needs to be updated to ToT to be appliable.

Comment on attachment 94964 [details] proposed fix View in context: https://bugs.webkit.org/attachment.cgi?id=94964&action=review > Tools/QtTestBrowser/launcherwindow.cpp:413 > + m_findBar = new QToolBar("Find"); Also, this should be QToolBar("Find", this) since otherwise you might leak m_findBar.

Created attachment 95135 [details] proposed fix

Thank you, that you viewed my patch With the enum you were right, I fixed it. Let me explain how the method works: When you type, or push the next or prev button it will find the text. If you change the checkbox's setting the right flag will be set up in the m_findFlag with a bitwise XOR. The HighLightAllOccurences is a special case, because when it set on, the find doesn't highlight the next founded text again so it has to make a new find without the HighLightAllOccurences flag to get the right result.

Comment on attachment 95135 [details] proposed fix View in context: https://bugs.webkit.org/attachment.cgi?id=95135&action=review > Tools/QtTestBrowser/launcherwindow.cpp:4 > + * Copyright (C) 2011 Kristof Kosztyo <Kosztyo.Kristof@stud.u-szeged.hu> Perhaps you can squeeze in one line but no big deal. > Tools/QtTestBrowser/launcherwindow.cpp:201 > + editMenu->addAction("&Find", this, SLOT(showFindBar()), QKeySequence(Qt::CTRL | Qt::Key_F)); On Mac this is not the normal shortcuts. Command is. > Tools/QtTestBrowser/launcherwindow.cpp:427 > + findClose->setText("X"); Nitpick : Can you find a nice close icon on google (Creative Commons) or something? QtTestBrowser has to be awesome looking :D.

Comment on attachment 95135 [details] proposed fix View in context: https://bugs.webkit.org/attachment.cgi?id=95135&action=review > Tools/QtTestBrowser/launcherwindow.cpp:1033 > + page()->findText("", QWebPage::HighlightAllOccurrences); some real browsers pre fill it with any selected content in the page. it could be a nice enhancement. > Tools/QtTestBrowser/launcherwindow.cpp:1037 > +void LauncherWindow::find(int mode = m_findNormal) looks ugly :) > Tools/QtTestBrowser/launcherwindow.h:249 > + static const int m_findNormal = 0; could we have a better name?

> > Tools/QtTestBrowser/launcherwindow.cpp:1037 > > +void LauncherWindow::find(int mode = m_findNormal) > > looks ugly :) > > > Tools/QtTestBrowser/launcherwindow.h:249 > > + static const int m_findNormal = 0; > > could we have a better name? I think it's better to be "s_findNormal". m_ prefix indicates member variable. Or maybe simply using 0. Also, I still find the logic inside find() function a bit confusing. It seems page()->findText() maybe be triggered several times in certain cases. And the following line seems a typo (isn't ~m_findNormal == 1?): found = page()->findText(m_lineEdit->text(), QFlag((m_findFlag | QWebPage::HighlightAllOccurrences) & ~m_findNormal)); Another thing I personally am not sure is the difference between LauncherWindow and MainWindow. It seems to be me the other toolbar is a member of MainWindow. Is there anything this implementation needs from LauncherWindow only?

(In reply to comment #10) > (From update of attachment 95135 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=95135&action=review > > > Tools/QtTestBrowser/launcherwindow.cpp:1033 > > + page()->findText("", QWebPage::HighlightAllOccurrences); > > some real browsers pre fill it with any selected content in the page. it could be a nice enhancement. :)) It is a good idea. But do we really want to make QtTestBrowser the best browser in this bug? :) If it is a simple fix, let's do it now. But if it isn't so trivial, fix it in a new bug report.

> I think it's better to be "s_findNormal". m_ prefix indicates member variable. > Or maybe simply using 0. I will cahnge it. > Also, I still find the logic inside find() function a bit confusing. It seems page()->findText() maybe be triggered several times in certain cases. And the following line seems a typo (isn't ~m_findNormal == 1?): > found = page()->findText(m_lineEdit->text(), QFlag((m_findFlag | QWebPage::HighlightAllOccurrences) & ~m_findNormal)); honestly I don't know why I did need to make like this, but it really doesn't needed I will delete that. > Another thing I personally am not sure is the difference between LauncherWindow and MainWindow. It seems to be me the other toolbar is a member of MainWindow. Is there anything this implementation needs from LauncherWindow only? The menu assembled here, it seems logical to place here.

> > Another thing I personally am not sure is the difference between LauncherWindow and MainWindow. It seems to be me the other toolbar is a member of MainWindow. Is there anything this implementation needs from LauncherWindow only? > > The menu assembled here, it seems logical to place here. Thanks, good to know.:)

(In reply to comment #12) > (In reply to comment #10) > > (From update of attachment 95135 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=95135&action=review > > > > > Tools/QtTestBrowser/launcherwindow.cpp:1033 > > > + page()->findText("", QWebPage::HighlightAllOccurrences); > > > > some real browsers pre fill it with any selected content in the page. it could be a nice enhancement. > > :)) > > It is a good idea. But do we really want to make QtTestBrowser the best browser in this bug? :) No no, unless you want to do it yourself :) > If it is a simple fix, let's do it now. But if > it isn't so trivial, fix it in a new bug report. Sure. Not even needed, just an idea if one likes to play with it...

Created attachment 95328 [details] proposed fix

Comment on attachment 95328 [details] proposed fix I think the patch is OK now. A reviewer will set the r flag for you.

Comment on attachment 95328 [details] proposed fix View in context: https://bugs.webkit.org/attachment.cgi?id=95328&action=review typo: findWarpAround -> findWrapAround > Tools/QtTestBrowser/launcherwindow.cpp:427 > + m_lineEdit = new QLineEdit(m_findBar); > + m_lineEdit->setMaximumWidth(200); > + QCheckBox* findCaseSensitive = new QCheckBox("CaseSensitive", m_findBar); > + QCheckBox* findWarpAround = new QCheckBox("WarpAround", m_findBar); > + QCheckBox* findHighLightAll = new QCheckBox("HighLightAll", m_findBar); > + QToolButton* findNext = new QToolButton(m_findBar); > + findNext->setArrowType(Qt::RightArrow); > + QToolButton* findPrevious = new QToolButton(m_findBar); > + findPrevious->setArrowType(Qt::LeftArrow); > + QToolButton* findClose = new QToolButton(m_findBar); > + findClose->setText("X"); It would be great to keep the natural order here too: findClose, m_lineEdit, findPrevious, findNext, findCaseSensitive, findWrapAround, findHighLightAll

Comment on attachment 95328 [details] proposed fix View in context: https://bugs.webkit.org/attachment.cgi?id=95328&action=review Took another look. I think the order and typo could be fixed during landing, but I have a few questions: > Tools/QtTestBrowser/launcherwindow.cpp:1040 > +void LauncherWindow::find(int mode = s_findNormalFlag) > +{ > + page()->findText("", QFlag(QWebPage::HighlightAllOccurrences)); Do we always need to call this? Is this for clearing the previous search? > Tools/QtTestBrowser/launcherwindow.cpp:1053 > + if (mode == s_findNormalFlag) > + page()->findText(m_lineEdit->text(), QFlag(m_findFlag & ~QWebPage::HighlightAllOccurrences)); > + else if (mode == QWebPage::FindBackward) > + page()->findText(m_lineEdit->text(), QFlag((m_findFlag | QWebPage::FindBackward) & ~QWebPage::HighlightAllOccurrences)); > + else > + m_findFlag = m_findFlag ^ mode; > + > + if (!m_lineEdit->text().isEmpty()) > + found = page()->findText(m_lineEdit->text(), QFlag(m_findFlag | QWebPage::HighlightAllOccurrences)); This seems to be needlessly complicated. Do we need all these findText calls for highlighting to work properly? Couldn't we just combine the proper find flags in the conditional cases and call findText once (or twice in case of HighlightAllOccurrences)?

Created attachment 95407 [details] proposed fix

Comment on attachment 95407 [details] proposed fix Great work. LGTM, Ossy what do you think?

Comment on attachment 95407 [details] proposed fix Great patch, r=me.

Comment on attachment 95407 [details] proposed fix Clearing flags on attachment: 95407 Committed r87720: <http://trac.webkit.org/changeset/87720>

All reviewed patches have been landed. Closing bug.

Revision r87720 cherry-picked into qtwebkit-2.2 with commit af58e95 <http://gitorious.org/webkit/qtwebkit/commit/af58e95>

"Buildfix for --minimal and CONFIG+=qt_minimal build" commited in r87724

Revision r87724 cherry-picked into qtwebkit-2.2 with commit cbc9e96 <http://gitorious.org/webkit/qtwebkit/commit/cbc9e96>