716464 - Typing about:Addons into the url bar loads the add-ons manager but doesn't hide the address bar (the test is case sensitive)

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[skr]

Attachment: ff-10b-addon-page-bug.png

User Agent: Mozilla/5.0 (X11; Linux i686) AppleWebKit/535.7 (KHTML, like Gecko) Chrome/16.0.912.75 Safari/535.7

Steps to reproduce:

typed about:Addons in address bar.


Actual results:

Addon page opened but address bar did not hide.


Expected results:

address bar should have disappeared as it happens with "about:addons".

Comment 1

[skr]

Tested positive on Firefox1.0.1(Windows7)

Comment 2

[skr]

(In reply to skr from comment #1)
> Tested positive on Firefox1.0.1(Windows7)

EDIT: Tested positive on Firefox7.0.1(Windows7)

Comment 3

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Over to the MSU team. 

See bug 571970 for where this bug was introduced. You'll need to modify the unit test (browser_disablechrome.js) to cover this, https://developer.mozilla.org/en/Browser_chrome_tests is useful reading on that test framework, which is the primary method of testing browser UI.

Comment 4

[MSU Capstone Team]

Attachment: disabled chrome for about: pages with capitalized letters

Second bug fixed by MSU Capstone Team; Please review;

Comment 5

[MSU Capstone Team]

Attachment: Real patch for this bug; Last one included testpage in it; Please review;

Real patch for this bug; Last one included testpage in it; Please review;

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 590286 [details] [diff] [review]
disabled chrome for about: pages with capitalized letters

Review of attachment 590286 [details] [diff] [review]:
-----------------------------------------------------------------

It looks like your work on adding a test about: page has found its way into this patch. You can do split these files out of your patch by following the instructions here: https://developer.mozilla.org/en/Mercurial_Queues#Splitting_a_patch.2C_the_easy_case:_per-file_splitting

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 590303 [details] [diff] [review]
Real patch for this bug; Last one included testpage in it; Please review;

Review of attachment 590303 [details] [diff] [review]:
-----------------------------------------------------------------

Please add a testcase to browser_disablechrome.js that tries to load the about:Addons url and confirms that the chrome is disabled.

Comment 8

[MSU Capstone Team]

Back then the sample code on chrome test tutorial page was not there somehow. Will walk through the tutorial again asap.

(In reply to Jared Wein [:jaws] from comment #7)
> Comment on attachment 590303 [details] [diff] [review]
> Real patch for this bug; Last one included testpage in it; Please review;
> 
> Review of attachment 590303 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please add a testcase to browser_disablechrome.js that tries to load the
> about:Addons url and confirms that the chrome is disabled.

Comment 9

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 590303 [details] [diff] [review]
Real patch for this bug; Last one included testpage in it; Please review;

Review of attachment 590303 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/browser.js
@@ +4758,5 @@
>    },
>  
>    hideChromeForLocation: function(aLocation) {
>      return this.inContentWhitelist.some(function(aSpec) {
> +      return aSpec.toLowerCase() == aLocation.toLowerCase();

It's safe to assume the strings in inContentWhitelist are already lowercase. Also, you should be lowercasing aLocation outside of the loop to avoid doing it multiple times.

Comment 10

[MSU Capstone Team]

Attachment: Bug 716464 Fix with added test as well

I believe that this should be the correct changes, the only changes reflected are in the browser.js and browser_disablechrome.js.  I just made all 4 of us the authors of this patch.

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 591220 [details] [diff] [review]
Bug 716464 Fix with added test as well

>--- a/browser/base/content/test/browser_disablechrome.js
>+++ b/browser/base/content/test/browser_disablechrome.js
>@@ -104,7 +104,7 @@
> 
> function test_url(aURL, aCanHide, aNextTest) {
>   is_chrome_visible();
>-
>+  

Can you remove the extra whitespace characters that were added here?

>@@ -152,6 +152,11 @@
>     TabsOnTop.enabled = false;
>     run_http_test_2();
>   });
>+  test_url("about:Addons", true, function() {
>+    info("Tabs on bottom");
>+    TabsOnTop.enabled = false;
>+    run_http_test_2();
>+  });
> }

We could probably add a test for both tabs-on-top and tabs-on-bottom, so two tests could be added. 

Instead of having run_chrome_about_test_2() call test_url with a third argument of end_test, we'll want to place a new test in there. Like so:

> // Should not hide the chrome
> function run_chrome_about_test_2() {
>   info("Chrome about: tests");
>   test_url("about:addons", true, run_http_test3);
> }
> 
> function run_http_test3() {
>   info("HTTP tests");
>   test_url(HTTPSRC + "disablechrome.html", false, run_chrome_about_test_3);
> }
> 
> // Should not hide the chrome
> function run_chrome_about_test_3() {
>   info("Chrome about: tests");
>   test_url("about:Addons", true, function() {
>     info("Tabs on top");
>     TabsOnTop.enabled = true;
>     run_http_test_4();
>   });
> }
> 
> function run_http_test4() {
>   info("HTTP tests");
>   test_url(HTTPSRC + "disablechrome.html", false, run_chrome_about_test_4);
> }
> 
> function run_chrome_about_test_4() {
>   info("Chrome about: tests");
>   test_url("about:Addons", true, end_test);
> }

Comment 12

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 591220 [details] [diff] [review]
Bug 716464 Fix with added test as well

Review of attachment 591220 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/browser.js
@@ +4711,4 @@
>        }
>  
>        // Show or hide browser chrome based on the whitelist
> +      if (this.hideChromeForLocation(location.toLowerCase()))

hideChromeForLocation() can potentially be used elsewhere - should be lower-casing inside that function (but outside the loop) to ensure the results are consistent, and avoid future bugs.

Comment 13

[MSU Capstone Team]

Attachment: Test fixed, lowercasing location changed to inside hideChromeForLocation

The changes from the last two comments have been made and we were able to run a successful test.  The url bar is disabled in the build we have compiled

Comment 14

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 591602 [details] [diff] [review]
Test fixed, lowercasing location changed to inside hideChromeForLocation

This looks great! I compiled your changes and also ran the tests. Thanks!

Comment 15

[Jared Wein [:jaws] (please needinfo? me)]

Pushed to fx-team:
https://hg.mozilla.org/integration/fx-team/rev/dca6fb65e46b

This will make it's way to the mozilla-central repository within a day or two.

Comment 16

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/dca6fb65e46b