1201340 - update aboutBase.css to include an explicit background color

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Logins, Passwords and Form Fill, defect)

Description

[Allison Naaktgeboren :ally]

It has come out that we expect our about page headers to have a specific background color. However we do not do that explicitly in aboutBase.css. We should.

the desired background color for headers is #F5F5F5

Comment 1

[Prateek Arora]

Hi, Can I take up this bug ? As I understood, we need background-color: #00ff00; in .header in aboutBase.css ? Right ?

Comment 2

[Prateek Arora]

Attachment: bug1201340.patch

Something like this ? Sorry for wrong color hex in previous comment.

Comment 3

[Allison Naaktgeboren :ally]

Yes you are welcome to pick up the bug. Please assign it to yourself. Is this your first bug? 

Yes, that's how to update aboutBase.css, so that part is correct. :)

The tricky bit is verifying the patch. That all the other about pages that consume .header behave correctly now that you've added it and that the test suite is still all correct. 

What I would do is:
1) find all the other about pages that consume the aboutBase's header class (many are in java, so you get to skip those. yay) and verify visually that all the headers all still look good.

2) run a try build and verify that there are no tests in the tree on mobile that break with the change.

To help you find places in the android code base that use the header class, we have dxr, https://dxr.mozilla.org/mozilla-central/source/mobile 

Are you familiar with try pushes & the tree? 

https://wiki.mozilla.org/ReleaseEngineering/TryServer#How_to_push_to_try
https://treeherder.mozilla.org/#/jobs?repo=fx-team


If you don't have or don't want try privileges, let me know and I can push the try run for you. If you have questions, please feel free to ask.

Comment 4

[Prateek Arora]

Hi,
This would be my 3rd bug. The first 2 were [good first bugs].

I will do as suggested and revert back asap. I don't have try priviledges, so you might have to do that for me. 
The bug is still assigned to you, How do I change that ?

Comment 5

[Allison Naaktgeboren :ally]

You can change the bugzilla assignee by logging into bugzilla, and clicking the edit link next to teh assignee field, then type in the new assignee.

Comment 6

[Allison Naaktgeboren :ally]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2f3b3a4caacc

You might want to consider doing the work to get try privileges sooner rather later. https://developer.mozilla.org/en-US/docs/Tools/Contributing#Pushing_to_try  

I used trychooser http://trychooser.pub.build.mozilla.org/ to add the syntax to the patch comment to test your patch on try.

Please use NEEDINFO if you need help with understanding your try run and looking for other about pages to check. It shows up in my bugzilla dashboard and helps me help you as promptly as possible. Thanks!

Comment 7

[Prateek Arora]

(In reply to Allison Naaktgeboren please NEEDINFO? :ally from comment #6)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=2f3b3a4caacc
> 
Seems like there were some failures. But not due to this bug right ? (I could see bug 1202045 in summary).

> You might want to consider doing the work to get try privileges sooner
> rather later.
> https://developer.mozilla.org/en-US/docs/Tools/Contributing#Pushing_to_try  
Will do that. I am still getting familiar with Mercurial and pushing etc. So I think I will wait till my 4th-5th bug. Want my voucher to be confident in me :) 

> I used trychooser http://trychooser.pub.build.mozilla.org/ to add the syntax
> to the patch comment to test your patch on try.
Thanks. Will have a look into it !

Comment 8

[Prateek Arora]

(In reply to Allison Naaktgeboren please NEEDINFO? :ally from comment #5)
> You can change the bugzilla assignee by logging into bugzilla, and clicking
> the edit link next to teh assignee field, then type in the new assignee.

Oh, but I couldn't see those fields editable until now when I was assigned.(No edit button in front of assignee field). May be didn't have permissions before. Do you think so ?

Comment 9

[Kevin Brosnan [Ex-Mozilla]]

Not being able to edit the assignee field is expected for new users. Given that you have fixed a couple bugs I have added you to the editbugs group which will allow you change almost all the fields of a bug besides previous comments.

Comment 10

[Allison Naaktgeboren :ally]

(In reply to Prateek Arora from comment #7)
> (In reply to Allison Naaktgeboren please NEEDINFO? :ally from comment #6)
> > https://treeherder.mozilla.org/#/jobs?repo=try&revision=2f3b3a4caacc
> > 
> Seems like there were some failures. But not due to this bug right ? (I
> could see bug 1202045 in summary).

One of the things I don't like about the treeherder UI is that its not obvious at all that the bug lists you see in the summary are _suggestions_ or possible matches. So the presence of those in the summary does not mean you're in the clear.

That mochitest 14 failed twice is a little concerning for your patch. I have re-triggered the test a bunch of times. If they all go orange, we have a problem.

The feature is used by sheriffs and others to mark intermittent test failures that shouldn't close the tree or backout code. We call this "starring the oranges"

How goes the looking for other fennec about pages that use the aboutBase header class?

Comment 11

[Allison Naaktgeboren :ally]

So all of the 14 runs have gone orange. This suggests there is a reproducible problem. Sometimes the test failure isn't caused directly by your patch, but your patch has tripped up some precondition or made a pre-existing race condition worse.

So far we've:
- made a patch and manually checked the fix
- run the build on the try server to check for test failures
- found a suspicious failure
- retriggered the failure 7x, so pretty consistent failure

So let's take a look: 
This is the raw log of the test failure
https://treeherder.mozilla.org/logviewer.html#?job_id=11373710&repo=try

the last test to start before the process crash is
13:43:12     INFO -  37 INFO TEST-START | editor/libeditor/tests/browserscope/test_richtext2.html

which is 
http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/tests/browserscope/test_richtext2.html

I usually open the test locally in my editor for a closer look

This test appears to be testing basic functionality like copy/paste so it seems an odd place to have a failure...unless it's copying from an about:page.
However I see nothing in this test that touches one of those.

So I give a local run of the failing test  https://wiki.mozilla.org/Mobile/Fennec/Android/Testing
./mach mochitest editor/libeditor/tests/browserscope/test_richtext2.html 

it passed. Sometimes though the test failure is actually caused by one of the _preceding_ tests. So, I'm going to run the whole test suite for libeditor. In general don't do this. It takes a really long time and is a pain. 
./mach mochitest editor/libeditor/tests/ 
..and it all passes.

So maybe we just pushed to try while something else was going down on the fx-team tree. https://treeherder.mozilla.org/#/jobs?repo=try&revision=ce1e682c45a5 and see if we even still have a problem.

Comment 12

[Allison Naaktgeboren :ally]

So that last try run, test suite #7 went orange, but then green (bless automatic retriggers). I've kicked off a couple more re-runs just to dot the is and cross the ts, but I think we're fine there. Your patch is not going to break the tree. 

So that just leaves the manual inspection portion. Which mobile about pages use aboutBase.css's header class, and thus are impacted by your change?

Comment 13

[Prateek Arora]

Woah, there was a mid-air collision :)

Here's what I wrote :

"Hi,
Yes the other about* pages are all good. I found aboutAddons, aboutDevices, aboutDownloads and aboutLogins to be using header from aboutBase.css. As you said, no need to check the java ones. So I think we are fine there. 

Nice info on the tests and failures above. I am lagging alot on tests I think, will catchup with the wiki links you provided. Thanks

Meanwhile, https://treeherder.mozilla.org/#/jobs?repo=try&revision=ce1e682c45a5 seemed to have failed !
This timed out -- dom/imptests/html/dom/nodes/test_Node-nodeName.xhtml ? also could see a red one in there. will try out this test locally..

What would be the next steps in this case ?"

Comment 14

[Prateek Arora]

So should I add check-in ?

Comment 15

[Allison Naaktgeboren :ally]

(In reply to Prateek Arora from comment #13)
> Woah, there was a mid-air collision :)
> 
> Here's what I wrote :
> 
> "Hi,
> Yes the other about* pages are all good. I found aboutAddons, aboutDevices,
> aboutDownloads and aboutLogins to be using header from aboutBase.css. As you
> said, no need to check the java ones. So I think we are fine there. 

This list is not necessarily for you and I, but for SoftVision, QA, who will verify this bug once the patch lands. Bugs also serve as records of STRs and what changes to expect when different versions ship, so it is important to be clear.


> 
> Nice info on the tests and failures above. I am lagging alot on tests I
> think, will catchup with the wiki links you provided. Thanks

In most cases, the thing that will hold you up is tests, and debugging them. Doesn't seem to matter what code base or what company. Since you are looking to be involved, getting up to speed on treeherder and test debugging will benefit you greatly.

> 
> Meanwhile,
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=ce1e682c45a5 seemed
> to have failed !
> This timed out -- dom/imptests/html/dom/nodes/test_Node-nodeName.xhtml ?
> also could see a red one in there. will try out this test locally..
> 
> What would be the next steps in this case ?"

You named it already, you check it out locally. You'll note that the red test when retriggered goes and stays green, so that is considered not the fault of your patch.

Comment 16

[Allison Naaktgeboren :ally]

(In reply to Prateek Arora from comment #14)
> So should I add check-in ?

Now that we've covered verification, you get an r+, and yes you can.

Mind https://treestatus.mozilla.org/ . No one can check patches in for you if the tree is closed. :)

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/ed3926c9ab28

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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