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

VERIFIED FIXED

Status

VERIFIED FIXED
7 years ago
7 years ago

People

(Reporter: whimboo, Assigned: vladmaniac)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mozmill-test-failure], URL)

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

7 years ago
+++ 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
The new tab page's title hasn't been touched for the new layout. No idea what's going wrong here.
(Reporter)

Comment 2

7 years ago
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?
(Assignee)

Comment 3

7 years ago
(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
(Reporter)

Comment 4

7 years ago
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
(Reporter)

Updated

7 years ago
Keywords: regression
(Assignee)

Updated

7 years ago
Assignee: nobody → vlad.mozbugs
Status: NEW → ASSIGNED
(Assignee)

Comment 5

7 years ago
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
(Reporter)

Comment 6

7 years ago
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.
(Assignee)

Comment 7

7 years ago
(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!
(Assignee)

Comment 8

7 years ago
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
(Assignee)

Comment 9

7 years ago
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
(Reporter)

Comment 10

7 years ago
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.
(Assignee)

Comment 11

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

Attaching my wip patch with a feedback request
Attachment #608258 - Flags: feedback?(hskupin)
(Assignee)

Updated

7 years ago
Attachment #608258 - Attachment is patch: true
(Reporter)

Comment 12

7 years ago
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.
Attachment #608258 - Flags: feedback?(hskupin) → feedback-
(Assignee)

Comment 13

7 years ago
> 
> >+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}}]
(Assignee)

Comment 14

7 years ago
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
Attachment #608258 - Attachment is obsolete: true
Attachment #608638 - Flags: feedback?(hskupin)
(Reporter)

Comment 15

7 years ago
So what's the value getProperty returns? the mentioned pastebin is also invalid because it uses browser.properties.
(Assignee)

Comment 16

7 years ago
(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
(Reporter)

Comment 17

7 years ago
So why should this patch fail then as mentioned in comment 14? I'm now totally confused.
(Assignee)

Comment 18

7 years ago
(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'
(Reporter)

Comment 19

7 years ago
(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.
(Reporter)

Comment 20

7 years ago
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.
Attachment #608638 - Flags: feedback?(hskupin) → review+
(Reporter)

Comment 21

7 years ago
Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/1dc599757315
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Keywords: regression
Resolution: --- → FIXED
Summary: Mozmill test failure /testTabbedBrowsing/testNewTab.js: Correct tab title - 'Neuer Tab' should equal 'New Tab' → Mozmill test failure /testTabbedBrowsing/testNewTab.js because DTD entity instead of property for new tab title is used
(Reporter)

Comment 22

7 years ago
Also has been transplanted to Aurora:
http://hg.mozilla.org/qa/mozmill-tests/rev/dce2d30854f1
(Assignee)

Comment 23

7 years ago
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.
(Reporter)

Comment 24

7 years ago
Looks good and is not failing anymore in latest testruns on localized builds.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.