Bug 107461 – [Qt] QtTestBrowser should provide option to enable/disable Javascript

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Qt

[Vivek Galatage]

QtTestBrowser should provide option to enable/disable Javascript.

Patch to follow.

[Vivek Galatage]

Created attachment 183804 [details]
Patch

[Vivek Galatage]

Created attachment 183888 [details]
Patch

[Noam Rosenthal]

You can do this via web inspector; Do you really need this duplicated in the browser settings?

[Vivek Galatage]

(In reply to comment #3)
> You can do this via web inspector; Do you really need this duplicated in the browser settings?

yes I agree. But for a quick test for disabling it we need to open the inspector and then navigate it to the "Inspector settings" and then in the "General" tab to actually disable it. I thought for a test browser that would take a little longer hence introduced this quick menu item. 

Also one thing I am finding different is that on linux the inspector is not shown inside the QtTestBrowser whereas the same is shown on the mac.

[Jocelyn Turcotte]

(In reply to comment #4)
It feels like you would be the only one using this menu. QtTestBrowser is more a WebKit developer tool then a web developer tool.

Do you see a use case where other developers might want to use this menu?

[Vivek Galatage]

(In reply to comment #5)
> (In reply to comment #4)
> It feels like you would be the only one using this menu. QtTestBrowser is more a WebKit developer tool then a web developer tool.
> 
> Do you see a use case where other developers might want to use this menu?

I agree. The use case is something like: I am working on [1] and wanted a quick way to verify this bug. Hence I wanted it to quickly provide me an option to enable/disable the Javascript and hence prompted me in adding it to the QtTestBrowser.

[1] https://bugs.webkit.org/show_bug.cgi?id=103902

[Vivek Galatage]

(In reply to comment #5)
> (In reply to comment #4)
> It feels like you would be the only one using this menu. QtTestBrowser is more a WebKit developer tool then a web developer tool.
> 
> Do you see a use case where other developers might want to use this menu?

Another use case is that of browsers (almost all the browsers) providing the option to disable JavaScript execution altogether. So this menu would help simulate that behavior as well. In this case, the users usually disable the option via browser settings as such.

[Jocelyn Turcotte]

Comment on attachment 183888 [details]
Patch

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

> Tools/QtTestBrowser/launcherwindow.cpp:501
> +    toggleJavaScriptEnabled->setChecked(true);

This should read:
toggleJavaScriptEnabled->setChecked(settings->testAttribute(QWebSettings::JavascriptEnabled));

> Tools/QtTestBrowser/launcherwindow.cpp:933
> +    statusBar()->showMessage(QString("Javascript ") + QString(enable ? "enabled" : "disabled"), 5000);

This isn't necessary, the checkbox already tells the user if JS is enabled or not.

[Vivek Galatage]

Comment on attachment 183888 [details]
Patch

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

Thank you for your feedback.

>> Tools/QtTestBrowser/launcherwindow.cpp:501
>> +    toggleJavaScriptEnabled->setChecked(true);
> 
> This should read:
> toggleJavaScriptEnabled->setChecked(settings->testAttribute(QWebSettings::JavascriptEnabled));

Sure, I will correct this.

>> Tools/QtTestBrowser/launcherwindow.cpp:933
>> +    statusBar()->showMessage(QString("Javascript ") + QString(enable ? "enabled" : "disabled"), 5000);
> 
> This isn't necessary, the checkbox already tells the user if JS is enabled or not.

Agreed.

[Vivek Galatage]

Created attachment 187057 [details]
Patch

[WebKit Review Bot]

Comment on attachment 187057 [details]
Patch

Clearing flags on attachment: 187057

Committed r142125: <http://trac.webkit.org/changeset/142125>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.