735883 - Mozmill test failure /testTabbedBrowsing/testNewTab.js because DTD entity instead of property for new tab title is used

Status: VERIFIED FIXED

Description

[Henrik Skupin (:whimboo)]

+++ This bug was initially created as a clone of Bug #724494 +++

Seems like this failure for localized versions of Firefox is back now. Have we had a recent change? Tim?

http://mozmill-ci.blargon7.com/#/functional/report/e438d6e3916b2b636037d77445cf20b0

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

The new tab page's title hasn't been touched for the new layout. No idea what's going wrong here.

Comment 2

[Henrik Skupin (:whimboo)]

So it looks like that we are still getting the wrong and not localized title. Vlad, can you please test this with the de version of Firefox?

Comment 3

[Maniac Vlad Florin (:vladmaniac)]

(In reply to Henrik Skupin (:whimboo) from comment #2)
> So it looks like that we are still getting the wrong and not localized
> title. Vlad, can you please test this with the de version of Firefox?

I don't get it, it worked. So the problem is in Germany :D I'll get to it and investigate

Comment 4

[Henrik Skupin (:whimboo)]

Worked with what version/locale of Firefox? I assume that you have only tested with the en-US build now. But it looks like that all localized builds are affected. Here for the ja one:

http://mozmill-ci.blargon7.com/#/functional/report/e438d6e3916b2b636037d77445d06fcf

Comment 5

[Maniac Vlad Florin (:vladmaniac)]

It seems that "newtab.pageTitle" equals "New Tab" instead of "Neuer Tab", which would be normal for the german locale - de. 

I checked the chrome page "chrome://browser/locale/newTab.dtd", got "New Tab"
Checked omni.jar and opened the file "NewTab.dtd" from ./firefox/chrome/de/locale/browser and got the same "New Tab" string value for newtab.pageTitle. 

Something is wrong here

Comment 6

[Henrik Skupin (:whimboo)]

So, it looks like what we need here is the property and not the DTD:

http://mxr.mozilla.org/l10n-central/source/de/browser/chrome/browser/tabbrowser.properties#18

I assume the title gets set by a script and it uses the property instead.

Comment 7

[Maniac Vlad Florin (:vladmaniac)]

(In reply to Henrik Skupin (:whimboo) from comment #6)
> So, it looks like what we need here is the property and not the DTD:
> 
> http://mxr.mozilla.org/l10n-central/source/de/browser/chrome/browser/
> tabbrowser.properties#18
> 
> I assume the title gets set by a script and it uses the property instead.

Right. Nice tip, thanks Henrik!

Comment 8

[Maniac Vlad Florin (:vladmaniac)]

Created attachment 607886 [details]
screenshot

Screenshot to show that I cannot see about:home in the "de" locally, and this causes my test to fail

Comment 9

[Maniac Vlad Florin (:vladmaniac)]

Changed the test to work with properties instead of reading the string from dtd 
This is the code snippet and the new error I get because of the about:home page 

I think this is not normal 
http://pastebin.mozilla.org/1529353

Comment 10

[Henrik Skupin (:whimboo)]

If you want to have a proper response you should really give us all the changed code. Not sure what the constant is set to.

Comment 11

[Maniac Vlad Florin (:vladmaniac)]

Created attachment 608258 [details] [diff] [review]
wip patch v1.0

Attaching my wip patch with a feedback request

Comment 12

[Henrik Skupin (:whimboo)]

Comment on attachment 608258 [details] [diff] [review]
wip patch v1.0

>+const TAB_TITLE_PROPERTY = "tabs.emptyTabTitle";

We never exposed this as constant so please put in the string directly into the function call.

>-  var title = utils.getEntity(tabBrowser.getDtds(), "newtab.pageTitle");
>+  var title = utils.getProperty("chrome://browser/locale/browser.properties", 
>+                                TAB_TITLE_PROPERTY);

I'm sure you want tabbrowser.properties here. That's the reason why it doesn't work for you.

Comment 13

[Maniac Vlad Florin (:vladmaniac)]

> 
> >+const TAB_TITLE_PROPERTY = "tabs.emptyTabTitle";
> 
> We never exposed this as constant so please put in the string directly into
> the function call.

Right. Thanks
> 
> >-  var title = utils.getEntity(tabBrowser.getDtds(), "newtab.pageTitle");
> >+  var title = utils.getProperty("chrome://browser/locale/browser.properties", 
> >+                                TAB_TITLE_PROPERTY);
> 
> I'm sure you want tabbrowser.properties here. That's the reason why it
> doesn't work for you.

I'm so sorry this patch was not refreshed. I was using tabbrowser.properties. If using browser.properties the error you get is 

"message": "getProperty: Unknown property - tabs.emptyTabTitle", "fileName": "resource://mozmill/stdlib/securable-module.js -> file:///home/vladmaniac/Desktop/germanlocale/mozmill-tests/lib/utils.js", "name": "Error", "lineNumber": 381}}]

Comment 14

[Maniac Vlad Florin (:vladmaniac)]

Created attachment 608638 [details] [diff] [review]
wip patch v1.1

With this patch you get the error I was talking about, meaning the about:home page causing problems on the german locale

Comment 15

[Henrik Skupin (:whimboo)]

So what's the value getProperty returns? the mentioned pastebin is also invalid because it uses browser.properties.

Comment 16

[Maniac Vlad Florin (:vladmaniac)]

(In reply to Henrik Skupin (:whimboo) from comment #15)
> So what's the value getProperty returns? the mentioned pastebin is also
> invalid because it uses browser.properties.

The property returns the expected result 'Neuer Tab'
http://pastebin.mozilla.org/1532035

Comment 17

[Henrik Skupin (:whimboo)]

So why should this patch fail then as mentioned in comment 14? I'm now totally confused.

Comment 18

[Maniac Vlad Florin (:vladmaniac)]

(In reply to Henrik Skupin (:whimboo) from comment #17)
> So why should this patch fail then as mentioned in comment 14? I'm now
> totally confused.

Henrik: 

The test fails on  this line 
expect.equal(tabBrowser.getTab().getNode().label, title, "Correct tab title");

tabBrowser.getTab().getNode().label value is 'Seiten-Ladefehler', because of about:home not working correctly, while title ahs the correct output: 'Neuer Tab'

Comment 19

[Henrik Skupin (:whimboo)]

(In reply to Maniac Vlad Florin (:vladmaniac) from comment #18)
> The test fails on  this line 
> expect.equal(tabBrowser.getTab().getNode().label, title, "Correct tab
> title");
> 
> tabBrowser.getTab().getNode().label value is 'Seiten-Ladefehler', because of
> about:home not working correctly, while title ahs the correct output: 'Neuer
> Tab'

Kadir, do you know why about:home doesn't work in de builds? I haven't tested on my own but as it looks like it's not a registered about: url.

Comment 20

[Henrik Skupin (:whimboo)]

Comment on attachment 608638 [details] [diff] [review]
wip patch v1.1

So I'm totally not sure what you are seeing here. Everything passes on my box. So I assume something is wrong with your installation. If you can reproduce it manually, even in a fresh profile, you should probably file a bug for Firefox.

This patch looks good and I will land it right away. Thanks.

Comment 21

[Henrik Skupin (:whimboo)]

Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/1dc599757315

Comment 22

[Henrik Skupin (:whimboo)]

Also has been transplanted to Aurora:
http://hg.mozilla.org/qa/mozmill-tests/rev/dce2d30854f1

Comment 23

[Maniac Vlad Florin (:vladmaniac)]

There was a problem with an earlier version of Firefox Nightly. Updated and everything is fine now. I take it there is no sense to raise a Firefox bug if the latest version behaves normally.

Comment 24

[Henrik Skupin (:whimboo)]

Looks good and is not failing anymore in latest testruns on localized builds.