Bug 101226 – [Qt] Open link in this window action

• Component: WebKit Qt
• Status: RESOLVED FIXED

[Allan Sandfeld Jensen]

When right clicking on a link that has a target specifying it should open in a new window we currently show two options that are functionally equivalent (open link (default) and open link in new window). KHTML in this case gives the user another option "open link in this window", which overrides the new window target. This is useful in several poorly designed webpages and a feature I personally miss in QtWebKit.

[Allan Sandfeld Jensen]

Created attachment 172341 [details]
Patch

[Allan Sandfeld Jensen]

Created attachment 172343 [details]
Patch

[WebKit Review Bot]

Attachment 172343 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebKit/qt/Api/qwebpage.cpp:2373:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/qt/Api/qwebpage.cpp:2374:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Total errors found: 2 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 172343 [details]
Patch

Attachment 172343 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14745157

[WebKit Review Bot]

Comment on attachment 172343 [details]
Patch

Attachment 172343 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14748054

[Peter Beverloo (cr-android ews)]

Comment on attachment 172343 [details]
Patch

Attachment 172343 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14731576

[Allan Sandfeld Jensen]

Created attachment 172351 [details]
Patch

[WebKit Review Bot]

Attachment 172351 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebKit/qt/Api/qwebpage.cpp:2373:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/qt/Api/qwebpage.cpp:2374:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Total errors found: 2 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jocelyn Turcotte]

Comment on attachment 172351 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=172351&action=review

> Source/WebCore/page/ContextMenuController.cpp:728
> +    ContextMenuItem OpenLinkInThisWindowItem(ActionType, ContextMenuItemTagOpenLinkInThisWindow,
> +        contextMenuItemTagOpenLinkInThisWindow());

I don't think that this is something that is beneficial for every QtWebKit application. It would be better not to add it to the default context menu and let application add it by overriding QWebView::contextMenuEvent.

> Source/WebKit/qt/Api/qwebpage.h:100
> +        OpenLinkInThisWindow,

Some documentation would be quite helpful given the ambiguity of the name.

[Allan Sandfeld Jensen]

(In reply to comment #8)
> If any of these errors are false positives, please file a bug against check-webkit-style.

The style bot discards comments, which makes it miscount the indentation. I could fix it by removing space between comments and arguments, but that does not look very nice.

(In reply to comment #9)
> (From update of attachment 172351 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=172351&action=review
> 
> > Source/WebCore/page/ContextMenuController.cpp:728
> > +    ContextMenuItem OpenLinkInThisWindowItem(ActionType, ContextMenuItemTagOpenLinkInThisWindow,
> > +        contextMenuItemTagOpenLinkInThisWindow());
> 
> I don't think that this is something that is beneficial for every QtWebKit application. It would be better not to add it to the default context menu and let application add it by overriding QWebView::contextMenuEvent.
> 
We could do that. That would just mean keeping the code in QtWebKit? The argument for making it more universal is only that OpenLink and OpenLinkInNewWindow are duplicates in this case, though we could also remove one of them by default.

> > Source/WebKit/qt/Api/qwebpage.h:100
> > +        OpenLinkInThisWindow,
> 
> Some documentation would be quite helpful given the ambiguity of the name.

Right, I missed that.

[Jocelyn Turcotte]

(In reply to comment #10)
> We could do that. That would just mean keeping the code in QtWebKit? The argument for making it more universal is only that OpenLink and OpenLinkInNewWindow are duplicates in this case, though we could also remove one of them by default.

I wish we could remove it but it is needed for code out there that implement QWebView::createWindow and expect the menu item to be there already.

[Allan Sandfeld Jensen]

Created attachment 172545 [details]
Patch

Do not populate the context-menu by default with the new action, but instead just add it in QtTestBrowser for testing

[WebKit Review Bot]

Attachment 172545 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebKit/qt/Api/qwebpage.cpp:2374:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/qt/Api/qwebpage.cpp:2375:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Total errors found: 2 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jocelyn Turcotte]

Comment on attachment 172545 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=172545&action=review

I think you also need to update editorCommandWebActions in qwebpage.cpp

Overall we usually have a team API review on each release to pass invidual APIs that were added through more eyes, maybe we should do this soon.

> Source/WebKit/qt/Api/qwebpage.h:100
> +        OpenLinkInThisWindow,

I think this needs to go at the end of the list for binary compatibility.

[Allan Sandfeld Jensen]

Created attachment 172795 [details]
Patch

Moved new enum to stay ABI compatible

[WebKit Review Bot]

Attachment 172795 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebKit/qt/Api/qwebpage.cpp:2374:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/qt/Api/qwebpage.cpp:2375:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Total errors found: 2 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Allan Sandfeld Jensen]

(In reply to comment #16)
> Attachment 172795 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
> Source/WebKit/qt/Api/qwebpage.cpp:2374:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
> Source/WebKit/qt/Api/qwebpage.cpp:2375:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
> Total errors found: 2 in 10 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

Right, we will get these until the patch for bug #101230 lands.

[Allan Sandfeld Jensen]

Created attachment 172803 [details]
Patch

Also updated editorCommentWebActions

[WebKit Review Bot]

Attachment 172803 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebKit/qt/Api/qwebpage.cpp:2375:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/qt/Api/qwebpage.cpp:2376:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Total errors found: 2 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Simon Hausmann]

Comment on attachment 172803 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=172803&action=review

> Source/WebKit/qt/Api/qwebpage.cpp:1698
> +    \value OpenLinkInThisWindow Open the current link without opening a new window. Used on links that would default to opening a new window. (Added in Qt 5.0)

"Used on links that would default to opening a new window" maybe that should also include frames? Like

"Used on links that would default to opening in another frame or a new window" ?

[Allan Sandfeld Jensen]

Committed r133763: <http://trac.webkit.org/changeset/133763>

[Csaba Osztrogonác]

(In reply to comment #21)
> Committed r133763: <http://trac.webkit.org/changeset/133763>

and it broke 28 tests: http://build.webkit.sed.hu/results/x86-32%20Linux%20Qt%20Release%20NRWT/r133763%20%2823083%29/results.html

Please fix the bug. Otherwise I suggest you should run tests before committing ...

[Allan Sandfeld Jensen]

(In reply to comment #22)
> (In reply to comment #21)
> > Committed r133763: <http://trac.webkit.org/changeset/133763>
> 
> and it broke 28 tests: http://build.webkit.sed.hu/results/x86-32%20Linux%20Qt%20Release%20NRWT/r133763%20%2823083%29/results.html
> 
> Please fix the bug. Otherwise I suggest you should run tests before committing ...

Sorry, it was last minute change. The addition to editorCommandWebAction is in the wrong place, and all the tests were run before that addition.

[Csaba Osztrogonác]

Fix landed in http://trac.webkit.org/changeset/133789.