1005635 - Remove reference to noise.png from browser/devtools/framework/connect/connect.css

Status: RESOLVED FIXED

(DevTools :: Framework, defect)

Description

[Tim Nguyen :ntim]

Now that we want to use a flat design. Noise.png is useless. It'd be nice to remove it.

Also, Brian, do you want to mentor this bug ?

Comment 1

[Brian Grinstead [:bgrins]]

References to the file: http://dxr.mozilla.org/mozilla-central/search?q=regexp%3Aurl\%28[%27%22]noise|url\%28[%27%22].*devtools\%2Fnoise&case=false.

There is a rule on the connect screen (chrome://browser/content/devtools/connect.xhtml) that can be removed, as it has a background color anyway.  Unfortunately, this would affect the UI on the app manager as well (I'll upload a screenshot to show).

Tim, would you want to remove the line in connect.css and upload it here?

Comment 2

[Brian Grinstead [:bgrins]]

Attachment: noise-bg.png

It's subtle, but the noise image is used here in the app manager (you can see it on the background on the left)

Comment 3

[Tim Nguyen :ntim]

(In reply to Brian Grinstead [:bgrins] from comment #1)
> References to the file:
> http://dxr.mozilla.org/mozilla-central/
> search?q=regexp%3Aurl\%28[%27%22]noise|url\%28[%27%22].
> *devtools\%2Fnoise&case=false.
> 
> There is a rule on the connect screen
> (chrome://browser/content/devtools/connect.xhtml) that can be removed, as it
> has a background color anyway.  Unfortunately, this would affect the UI on
> the app manager as well (I'll upload a screenshot to show).
> 
> Tim, would you want to remove the line in connect.css and upload it here?

Sure, but the goal of this bug was for new developers to get the opportunity to do it :)

Anyways, found another file that needs to be removed, so I might as well do it.

Comment 4

[Tim Nguyen :ntim]

(In reply to Tim Nguyen [:ntim] from comment #3)
> (In reply to Brian Grinstead [:bgrins] from comment #1)
> > References to the file:
> > http://dxr.mozilla.org/mozilla-central/
> > search?q=regexp%3Aurl\%28[%27%22]noise|url\%28[%27%22].
> > *devtools\%2Fnoise&case=false.
> > 
> > There is a rule on the connect screen
> > (chrome://browser/content/devtools/connect.xhtml) that can be removed, as it
> > has a background color anyway.  Unfortunately, this would affect the UI on
> > the app manager as well (I'll upload a screenshot to show).
> > 
> > Tim, would you want to remove the line in connect.css and upload it here?
> 
> Sure, but the goal of this bug was for new developers to get the opportunity
> to do it :)
> 
> Anyways, found another file that needs to be removed, so I might as well do
> it.

(background-noise-toolbar.png)

Comment 5

[vikneshwar]

Hey Tim , 
     Is it enough to remove noise.png reference or even background-noise-toolbar.png ? Thank you :)

Comment 6

[Tim Nguyen :ntim]

(In reply to vikneshwar from comment #5)
> Hey Tim , 
>      Is it enough to remove noise.png reference or even
> background-noise-toolbar.png ? Thank you :)

Yeah, just remove both of them,  but only in the devtools/ directory.
Other files with those names shouldn't be removed if they're not in the devtools/ directory.

I'm guessing you want this assigned to you ?

Comment 7

[Brian Grinstead [:bgrins]]

Actually, I don't think either of these files can be fully removed.  

background-noise-toolbar is used for codemirror:
http://dxr.mozilla.org/mozilla-central/search?q=regexp%3Aurl\%28[%27%22]background-noise-toolbaar|url\%28[%27%22].*devtools\%2Fbackground-noise-toolbar&redirect=true.

The only thing I think we can remove is the reference to noise.png from browser/devtools/framework/connect/connect.css.

Comment 8

[vikneshwar]

Attachment: noise.patch

Comment 9

[Brian Grinstead [:bgrins]]

Comment on attachment 8420949 [details] [diff] [review]
noise.patch

Review of attachment 8420949 [details] [diff] [review]:
-----------------------------------------------------------------

Can you add r=bgrins at the end of the patch commit message?

Comment 10

[vikneshwar]

Attachment: remove-noise-refrence.patch

Since its feedback for the previous patch , i had doubt about adding review to the commit message :)

Comment 11

[Ryan VanderMeulen [:RyanVM]]

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

Thanks for the patch! One request, can you please make sure that future patches include all of the necessary commit information when requesting checkin? Thanks!
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Comment 12

[Ryan VanderMeulen [:RyanVM]]

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

Comment 13

[Tim Nguyen :ntim]

Why should background-noise-toolbar.png stay for the devtools ? CodeMirror only used it for it's findbar to match the older theme. Now that we made our UI flat, we should get rid of it right ?

Comment 14

[Brian Grinstead [:bgrins]]

(In reply to Tim Nguyen [:ntim] from comment #13)
> Why should background-noise-toolbar.png stay for the devtools ? CodeMirror
> only used it for it's findbar to match the older theme. Now that we made our
> UI flat, we should get rid of it right ?

I guess so, I don't see much of a difference with or without it.  We could possibly use that as an opportunity to do a better job of theming the 'find' popup it to match the light theme (adding a theme-light, theme-dark rule for .CodeMirror-dialog).   Want to open a new bug for it?