Closed Bug 633772 Opened 9 years ago Closed 9 years ago

Web Console cleanup: Use of inheritance to set icons on hud-msg-nodes

Categories

(Toolkit :: Themes, defect)

x86
All
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla7

People

(Reporter: ShareBird, Assigned: ShareBird)

References

Details

(Whiteboard: [fixed-in-devtools][merged-to-mozilla-central])

Attachments

(1 file, 1 obsolete file)

I'm just wondering why we don't make use of inheritance to set those icons instead of setting them for the webconsole-msg-icon.

This would simplify the code making use of child selectors unnecessary.

Something like:
.webconsole-msg-network.webconsole-msg-error {
  -moz-image-region: rect(0, 16px, 8px, 8px);
}

instead of:
.webconsole-msg-network.webconsole-msg-error > .webconsole-msg-icon-container > .webconsole-msg-icon {
  -moz-image-region: rect(0, 16px, 8px, 8px);
}
Attached patch proposed patch (obsolete) — Splinter Review
Assignee: nobody → pardal
Assignee: pardal → nobody
Component: Developer Tools → Themes
Product: Firefox → Toolkit
QA Contact: developer.tools → themes
Attachment #512000 - Flags: review?(gavin.sharp)
Attachment #512000 - Flags: review?(gavin.sharp) → review?(dao)
Comment on attachment 512000 [details] [diff] [review]
proposed patch

I couldn't get the patch to apply. I manually removed the CRs, but the pinstripe part still failed.
Attachment #512000 - Flags: review?(dao) → review-
(In reply to comment #2)
> Comment on attachment 512000 [details] [diff] [review]
> proposed patch
> 
> I couldn't get the patch to apply. I manually removed the CRs, but the
> pinstripe part still failed.

Dão, I have no idea why does it fail at all... I have to say I'm not building Firefox myself and I'm using Windows tools to write the patch: Notepad++ and WinMerge.
I will describe what I've done, maybe I'm doing something completely wrong...

1. I take a copy from the hg row file and paste it on Notepad++. I choose to format in UNIX.
2. I take a copy from this file to make my modifications.
3. I compare the two files with WinMerge and use the tool to make a patch using format: Style=Unified and Context=7 (WinMerge doesn't allow me to choose 8 here)

Am I missing something?

Anyway, thank you very much!
New patch with updated pinstripe part. Sorry, it seems I've got the copy from MXR instead of hg...
Attachment #512000 - Attachment is obsolete: true
Attachment #513690 - Flags: review?(dao)
Attachment #513690 - Flags: review?(dao) → review+
Keywords: checkin-needed
Assignee: nobody → pardal
Status: NEW → ASSIGNED
this patch cannot be pushed, it doesn't have approval.
Please properly ask for approval with risk/gain rationale.
Keywords: checkin-needed
Whiteboard: [has-patch][reviewed][needs-approval]
I've totally forgot about this bug, actually I thought it was pushed a long time ago. Who and how should I request approval on it?
It doesn't need approval.
Keywords: checkin-needed
Whiteboard: [has-patch][reviewed][needs-approval]
I'm landing in devtools. thanks for the reminder!
Keywords: checkin-needed
Whiteboard: [fixed-in-devtools]
Comment on attachment 513690 [details] [diff] [review]
[in-devtools][checked-in] patch_v2

http://hg.mozilla.org/projects/devtools/rev/c4bd2219aafc
Attachment #513690 - Attachment description: patch_v2 → [in-devtools] patch_v2
Thank you Dão and Rob!
I've noticed we are doing the same on the Addons Manager. Should I file a bug and provide a patch also for it?
Sure, if it simplifies code.
(In reply to comment #10)
> Thank you Dão and Rob!
> I've noticed we are doing the same on the Addons Manager. Should I file a
> bug and provide a patch also for it?

Absolutely! Thanks for the patch. :)
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-devtools] → [fixed-in-devtools][merged-to-mozilla-central]
Target Milestone: --- → mozilla7
Comment on attachment 513690 [details] [diff] [review]
[in-devtools][checked-in] patch_v2

http://hg.mozilla.org/mozilla-central/rev/c4bd2219aafc
Attachment #513690 - Attachment description: [in-devtools] patch_v2 → [in-devtools][checked-in] patch_v2
You need to log in before you can comment on or make changes to this bug.