1190961 - Change info-pages.css' placeholder icon to one that exists in toolkit/

Status: RESOLVED FIXED

(Toolkit :: Themes, defect)

Description

[Tim Nguyen :ntim]

See https://bugzilla.mozilla.org/show_bug.cgi?id=1087618#c76 .

We should use a file that exists on toolkit's side.
chrome://global/skin/icons/warning.svg seems like a good candidate.

Comment 1

[Tim Nguyen :ntim]

In [0], chrome://browser/skin/aboutNetError_info.svg should be replaced with chrome://global/skin/icons/warning.svg


[0] : https://dxr.mozilla.org/mozilla-central/source/toolkit/themes/shared/in-content/info-pages.inc.css?from=info-pages.inc.css&offset=0#34

Comment 2

[Tummala Dhanvi [:c0mrad3][UTC+5.30]]

Hey I would like to work on this bug can You please assign this one to me

Comment 3

[Tummala Dhanvi [:c0mrad3][UTC+5.30]]

Attachment: Bug-1190961.patch

Comment 4

[Tim Nguyen :ntim]

Comment on attachment 8649939 [details] [diff] [review]
Bug-1190961.patch

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

The patch looks good to me ! Thanks for the patch :)

I'm not a peer, so my r+ doesn't have much value, so I'm going to ask a peer to review this as well.
Can you change r=ntim to r=jaws,ntim to reflect the peer review ?

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8649939 [details] [diff] [review]
Bug-1190961.patch

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

Thanks for the patch, but this changes the icon from a info (i) icon to a warning triangle /!\

I think we should just move the info icon from browser to toolkit.

Comment 6

[Tummala Dhanvi [:c0mrad3][UTC+5.30]]

(In reply to (Mostly away 9/11-9/23) Jared Wein [:jaws] (please needinfo? me) from comment #5)
> Comment on attachment 8649939 [details] [diff] [review]
> Bug-1190961.patch
> 
> Review of attachment 8649939 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for the patch, but this changes the icon from a info (i) icon to a
> warning triangle /!\
> 
> I think we should just move the info icon from browser to toolkit.

I will send a patch again and try to fix it :) I will read the comments and understand the thing that I need to do :)

Comment 7

[Tummala Dhanvi [:c0mrad3][UTC+5.30]]

So my target is to just move the info icon from browser to toolkit.

Comment 8

[Tummala Dhanvi [:c0mrad3][UTC+5.30]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #5)
> Comment on attachment 8649939 [details] [diff] [review]
> Bug-1190961.patch
> 
> Review of attachment 8649939 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for the patch, but this changes the icon from a info (i) icon to a
> warning triangle /!\
that what was mentioned in comment 1, so I did the same
> 
> I think we should just move the info icon from browser to toolkit.

so should I just move (did you mean move (mv) or copy ?) the icon ? 

what is the paths that I need to move ? 

also the icon might be used by some other files too na ? so is it a good idea to move the file ?

also if we copy the same file it increases the size of the source code too ?

Comment 9

[Tummala Dhanvi [:c0mrad3][UTC+5.30]]

hey Tim Nguyen can you please guide me with fixing this bug ?

Comment 10

[Tim Nguyen :ntim]

(In reply to Tummala Dhanvi [:c0mrad3][UTC+5.30] from comment #9)
> hey Tim Nguyen can you please guide me with fixing this bug ?

Sorry for the huge delay.
You'll need to:
- Move aboutNetError_info.svg to toolkit/themes/shared/incontent-icons and rename it to info.svg
(You can do `hg rename browser/themes/shared/aboutNetError_info.svg toolkit/themes/shared/incontent-icons/info.svg`)
- Remove the reference to aboutNetError_info.svg from `browser/themes/shared/jar.inc.mn`
- Replace all the references to aboutNetError_info.svg to point to toolkit. The new path will be `chrome://global/skin/icons/info.svg`.
- Add a manifest entry to toolkit/themes/shared/jar.inc.mn for the new info.svg file, based on [0]
- At [1], you'll need to change the url to `chrome://global/skin/icons/info.svg`


[0]: https://hg.mozilla.org/mozilla-central/file/d53a52b39a95/toolkit/themes/shared/jar.inc.mn#l22
[1]: https://dxr.mozilla.org/mozilla-central/source/toolkit/themes/shared/in-content/info-pages.inc.css?from=info-pages.inc.css&offset=0#34

Comment 11

[Tim Nguyen :ntim]

Do you need additional info ?

Comment 12

[Tummala Dhanvi [:c0mrad3][UTC+5.30]]

okay I think I have enough information to fix this bug :) 

I will attach a patch soon

Comment 13

[Tummala Dhanvi [:c0mrad3][UTC+5.30]]

Attachment: Bug-1190961-v2.patch

Hope I have done it correct :)

Comment 14

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8684552 [details] [diff] [review]
Bug-1190961-v2.patch

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

This is close, but the SVG file is still referenced by aboutNetError.css and aboutSocialError.css. Both of those places will need to be updated as well. See http://mxr.mozilla.org/mozilla-central/search?string=aboutNetError_info.svg

Comment 15

[Tummala Dhanvi [:c0mrad3][UTC+5.30]]

Attachment: Bug-1190961-v2.patch

Hope I am done this time :)

Comment 16

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8686419 [details] [diff] [review]
Bug-1190961-v2.patch

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

Looks good, thanks! And sorry for the delay on the review.

Comment 17

[Jared Wein [:jaws] (please needinfo? me)]

https://hg.mozilla.org/integration/fx-team/rev/5cb637976cf157ccbe39795017eba8f937aae9d5
Bug 1190961 - Change info-pages.css' placeholder icon to one that exists in toolkit/ r=jaws

Comment 18

[Tim Nguyen :ntim]

Please do not set the resolved fixed status, it will be set by the sheriffs once the patch lands in mozilla-central.

Comment 19

[Tummala Dhanvi [:c0mrad3][UTC+5.30]]

okay!

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5cb637976cf1