565148 - Add a helper method to UtilsAPI to get the default homepage

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[u279076]

As was discovered in Bug 559881, we need a helper method in UtilsAPI to get the default homepage.  This is a tracking bug for development of that helper method.

Comment 1

[u279076]

Here is the specifics of what this helper method should do:

So my suggestion is that we should create another helper function in the
UtilsAPI, which can retrieve us the default homepage. It has to look for a
resource url in browser.startup.homepage first. If it is present load it's
value, otherwise use the one in this pref.

Comment 2

[u279076]

Attachment: Patch v1

This patch does the following:

* Adds a getDefaultHomepage() method to the UtilsAPI
* Updates tests which use var defaultHomePage = UtilsAPI.getProperty() to use the new API method instead:
** testUntrustedConnectionErrorPage
** testSafeBrowsingNotificationBar
** testRestoreHomepageToDefault

Coincidentally, this patch fixes the failure in bug 559881 (testUntrustedConnectionErrorPage) and a failure in testRestoreHomepageToDefault (no bug report on file).

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 444777 [details] [diff] [review]
Patch v1

>+  var defaultHomepage = UtilsAPI.getDefaultHomepage();
>+  var browserHomepage = new elementslib.ID(controller.window.document, "browserHomePage");
>+  controller.assertValue(browserHomepage, defaultHomepage);

Please fold in the function directly into the assert call. There is no need for a separate variable anymore. Same applies to all other tests.

>+function getDefaultHomepage() {

Please alphabetical ordering for the functions in that module.

>+  // Get the browser.startup.homepage pref from about:config
>+  var defaultHomepage = prefs.getPref("browser.startup.homepage", 
>+                                      "resource:/browserconfig.properties");

Use an empty string as the default value. Otherwise it could cause problems for other applications.

>+  // A Regular Expression to check for the instance of ".properties"
>+  var regEx = /\.properties$/;
>+  
>+  // If the pref ends with a .properties then load the pref from 
>+  // the properties file
>+  if (regEx.test(defaultHomepage)) {

Please move the regEx definition directly into the if clause. Also please explain why you are testing for ".properties" instead of the resource protocol. The latter one is much safer because we can only use getProperty for a file which uses the resource protocol.

Comment 4

[u279076]

(In reply to comment #3)
> Also please
> explain why you are testing for ".properties" instead of the resource protocol.
> The latter one is much safer because we can only use getProperty for a file
> which uses the resource protocol.
I've already explained this to you a couple of times.  I'll state it here again:

On Windows, Mac, and Linux versions that don't alter Firefox, resource:/ is fine.  However, on Ubuntu we have to deal with the Ubuntu Modifications add-on (as you are well aware).  On Ubuntu when Ubuntu Modifications is enabled, the browser.startup.homepage pref comes from an ubufox.properties file located at a chrome:/ URL (not a resource:/ URL).  By checking for resource:/ we will miss the ubufox.properties file (which is the root of why we filed this bug in the first place).

If you want me to explain this in comments within the test, I can do that, but please state that's what you want.  

All other revisions above are no problem; I'll work on those today.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #4)
> I've already explained this to you a couple of times.  I'll state it here
> again:

I love to have those comments in bugs and not in conversations on other channels. While we were sitting on it last week I had a couple of other people to me and I was working on other things, so I lost it. But thanks for the explanation. It makes sense now.

> If you want me to explain this in comments within the test, I can do that, but
> please state that's what you want.  

That would also be fine for the future. Please add a short comment there or reference this bug, which would even better.

Comment 6

[u279076]

Attachment: Patch v2

Comment 7

[u279076]

Attachment: Patch v3

I found one more test which was using the old method: testSafeBrowsingWarningPages.js

This patch updates the test to use the helper method.  I used the following command to identify all tests:

grep -R -i "defaultHomePage = UtilsAPI.getProperty" ./

So to summarize, this patch adds the helper method to the API and fixes to the following tests:
 * testRestoreHomepageToDefault
 * testSafeBrowsingNotificationBar
 * testSafeBrowsingWarningPages
 * testUntrustedConnectionErrorPage

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 446398 [details] [diff] [review]
Patch v3

While this patch is fine, it doesn't really fix the tests because of the redirect. Lemme file a new bug for that issue.

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Landed on all branches as:
http://hg.mozilla.org/qa/mozmill-tests/rev/9a37e4f108a0
http://hg.mozilla.org/qa/mozmill-tests/rev/cddb043a1700
http://hg.mozilla.org/qa/mozmill-tests/rev/f5bb8245fadf

Anthony, can you please take care of the documentation on MDC? Thanks.

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #9)
> Anthony, can you please take care of the documentation on MDC? Thanks.

Would be good to get this function added to our docs.

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Move of Mozmill Test related project bugs to newly created components. You can
filter out those emails by using "Mozmill-Tests-to-MozillaQA" as criteria.