Bug 61425

Summary: [Qt] Implement find feature for QtTestBrowser
Product: WebKit Reporter: Kristóf Kosztyó <kkristof>
Component: Tools / TestsAssignee: Kristóf Kosztyó <kkristof>
Status: RESOLVED FIXED    
Severity: Normal CC: abecsi, ademar, commit-queue, cshu, ossy, tonikitoo
Priority: P2 Keywords: Qt, QtTriaged
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
proposed fix
none
proposed fix
ossy: review-, ossy: commit-queue-
proposed fix
none
proposed fix
none
proposed fix none

Description Kristóf Kosztyó 2011-05-25 00:37:02 PDT
[Qt] Implement find feature for QtTestBrowser
Comment 1 Kristóf Kosztyó 2011-05-26 06:05:48 PDT
Created attachment 94963 [details]
proposed fix
Comment 2 Kristóf Kosztyó 2011-05-26 06:09:19 PDT
Created attachment 94964 [details]
proposed fix
Comment 3 Chang Shu 2011-05-26 06:26:29 PDT
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 4 Andras Becsi 2011-05-26 06:28:20 PDT
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 5 Csaba Osztrogonác 2011-05-26 06:32:24 PDT
Comment on attachment 94964 [details]
proposed fix

r- because of the comments and it needs to be updated to ToT to be appliable.
Comment 6 Andras Becsi 2011-05-26 07:57:47 PDT
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.
Comment 7 Kristóf Kosztyó 2011-05-27 01:08:12 PDT
Created attachment 95135 [details]
proposed fix
Comment 8 Kristóf Kosztyó 2011-05-27 01:24:47 PDT
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 9 Alexis Menard (darktears) 2011-05-27 05:18:18 PDT
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 10 Antonio Gomes 2011-05-27 05:46:57 PDT
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?
Comment 11 Chang Shu 2011-05-27 07:00:19 PDT
> > 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?
Comment 12 Csaba Osztrogonác 2011-05-27 07:36:12 PDT
(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.
Comment 13 Kristóf Kosztyó 2011-05-27 09:28:25 PDT
> 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.
Comment 14 Chang Shu 2011-05-27 10:27:30 PDT
> > 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.:)
Comment 15 Antonio Gomes 2011-05-27 12:07:46 PDT
(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...
Comment 16 Kristóf Kosztyó 2011-05-30 00:48:44 PDT
Created attachment 95328 [details]
proposed fix
Comment 17 Andras Becsi 2011-05-30 01:47:52 PDT
Comment on attachment 95328 [details]
proposed fix

I think the patch is OK now.
A reviewer will set the r flag for you.
Comment 18 Csaba Osztrogonác 2011-05-30 07:09:22 PDT
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 19 Andras Becsi 2011-05-30 07:39:30 PDT
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)?
Comment 20 Kristóf Kosztyó 2011-05-31 02:07:02 PDT
Created attachment 95407 [details]
proposed fix
Comment 21 Andras Becsi 2011-05-31 02:15:36 PDT
Comment on attachment 95407 [details]
proposed fix

Great work. LGTM, Ossy what do you think?
Comment 22 Csaba Osztrogonác 2011-05-31 02:17:13 PDT
Comment on attachment 95407 [details]
proposed fix

Great patch, r=me.
Comment 23 WebKit Commit Bot 2011-05-31 03:42:13 PDT
Comment on attachment 95407 [details]
proposed fix

Clearing flags on attachment: 95407

Committed r87720: <http://trac.webkit.org/changeset/87720>
Comment 24 WebKit Commit Bot 2011-05-31 03:42:19 PDT
All reviewed patches have been landed.  Closing bug.
Comment 25 Ademar Reis 2011-05-31 07:03:55 PDT
Revision r87720 cherry-picked into qtwebkit-2.2 with commit af58e95 <http://gitorious.org/webkit/qtwebkit/commit/af58e95>
Comment 26 Ademar Reis 2011-05-31 11:49:04 PDT
"Buildfix for --minimal and CONFIG+=qt_minimal build" commited in r87724
Comment 27 Ademar Reis 2011-05-31 11:49:38 PDT
Revision r87724 cherry-picked into qtwebkit-2.2 with commit cbc9e96 <http://gitorious.org/webkit/qtwebkit/commit/cbc9e96>