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

RESOLVED FIXED in mozilla12

Status

()

Toolkit
Add-ons Manager
--
trivial
RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: skr, Assigned: MSU Capstone Team)

Tracking

10 Branch
mozilla12
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mentor=bmcbride@mozilla.com])

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

7 years ago
Created attachment 586907 [details]
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".
Component: Untriaged → Add-ons Manager
Product: Firefox → Toolkit
QA Contact: untriaged → add-ons.manager
Severity: normal → trivial
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: visible address bar on addon page → Typing about:Addons into the url bar loads the add-ons manager but doesn't hide the address bar (the test is case sensitive)
(Reporter)

Comment 1

7 years ago
Tested positive on Firefox1.0.1(Windows7)
(Reporter)

Comment 2

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

EDIT: Tested positive on Firefox7.0.1(Windows7)
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.
Assignee: nobody → owenccarpenter
Status: NEW → ASSIGNED
OS: Linux → All
Hardware: x86 → All
Whiteboard: [mentor=bmcbride@mozilla.com]
(Assignee)

Comment 4

7 years ago
Created attachment 590286 [details] [diff] [review]
disabled chrome for about: pages with capitalized letters

Second bug fixed by MSU Capstone Team; Please review;
Attachment #590286 - Flags: review?
(Assignee)

Comment 5

7 years ago
Created attachment 590303 [details] [diff] [review]
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;
Attachment #590303 - Flags: review?(jwein)
Attachment #590303 - Flags: review?(bmcbride)
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
Attachment #590286 - Flags: review?
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.
Attachment #590303 - Flags: review?(jwein)
(Assignee)

Comment 8

7 years ago
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.
Attachment #590286 - Attachment is obsolete: true
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.
Attachment #590303 - Flags: review?(bmcbride) → review-
(Assignee)

Comment 10

7 years ago
Created attachment 591220 [details] [diff] [review]
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.
Attachment #591220 - Flags: review?(bmcbride)
Attachment #590303 - Attachment is obsolete: true
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);
> }
Attachment #591220 - Attachment is patch: true
Attachment #591220 - Flags: feedback-
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.
Attachment #591220 - Flags: review?(bmcbride) → review-
(Assignee)

Comment 13

7 years ago
Created attachment 591602 [details] [diff] [review]
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
Attachment #591602 - Flags: review?(bmcbride)
Attachment #591220 - Attachment is obsolete: true
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!
Attachment #591602 - Attachment is patch: true
Attachment #591602 - Flags: feedback+
Attachment #591602 - Flags: review?(bmcbride) → review+
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.
Whiteboard: [mentor=bmcbride@mozilla.com] → [mentor=bmcbride@mozilla.com][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/dca6fb65e46b
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Whiteboard: [mentor=bmcbride@mozilla.com][fixed-in-fx-team] → [mentor=bmcbride@mozilla.com]
Target Milestone: --- → mozilla12

Updated

5 years ago
Duplicate of this bug: 630951
You need to log in before you can comment on or make changes to this bug.