Closed Bug 1434018 Opened 4 years ago Closed 4 years ago

Remove references to non-existent overlay

Categories

(Core :: Layout, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: bdahl, Assigned: dvabhinav31)

References

Details

Attachments

(1 file, 2 obsolete files)

There are a few references in layout-debug/ to tasksOverlay.xul which no longer exists.
I would like to work on this bug. Can you assign this to me?
Attached patch bug-1434018.patch (obsolete) — Splinter Review
I have uploaded the patch. I have made all the changes
Attachment #8946605 - Flags: review?(bdahl)
Comment on attachment 8946605 [details] [diff] [review]
bug-1434018.patch

I'm not a peer or owner the layout module. You'll need someone from this list: https://wiki.mozilla.org/Modules/All#Layout_Engine
Attachment #8946605 - Flags: review?(bdahl)
Assignee: nobody → dvabhinav31
Comment on attachment 8946605 [details] [diff] [review]
bug-1434018.patch

Forwarding the review request along...
Attachment #8946605 - Flags: review?(dbaron)
It looks like globalOverlay.xul in layout/tools/layout-debug/ui/content/layoutdebug.xul is not a valid file. Can you remove that too?
Flags: needinfo?(dvabhinav31)
Comment on attachment 8946605 [details] [diff] [review]
bug-1434018.patch

So the existing layout-debug code has overlays into both Firefox and SeaMonkey.  I think this is mostly removing the SeaMonkey overlay, but not completely.  There is, for example, the code lower down in layoutdebug-overlay.xul that should also be removed.

(I'm not sure if anyone's tested in the SeaMonkey overlay works for a decade or so... but there are certainly more references to tasksOverlay.xul in comm-central, although I don't actually see a *file* called tasksOverlay.xul, so I'm guessing it doesn't actually do anything anymore.)

So I think I'm fine with this if you also remove the other bit of the SeaMonkey overlay code in layoutdebug-overlay.xul.  But I'd like to look at the revised patch, so marking as review-.
Attachment #8946605 - Flags: review?(dbaron) → review-
... and also comment 5.
Attached patch bug-143018-v2.patch (obsolete) — Splinter Review
I have made all the mentioned changes.
Flags: needinfo?(dvabhinav31)
Attachment #8948198 - Flags: review?(dbaron)
Comment on attachment 8948198 [details] [diff] [review]
bug-143018-v2.patch

So this looks good, except for two things.  First, you should have a commit message that's suitable for committing to our repository.  For some recent examples of commit messages, see:
https://hg.mozilla.org/mozilla-central/pushloghtml .  In particular, you should have:
 * the bug number
 * a brief description of the code change being made
 * the reviewer
so something like:
Bug 1434018 - Remove references to old (SeaMonkey) overlays from layoutdebug code.  r=dbaron

> <?xml-stylesheet href="chrome://communicator/skin/" type="text/css" ?>
> 
>-<?xul-overlay href="chrome://global/content/globalOverlay.xul"?>
>-<?xul-overlay href="chrome://communicator/content/tasksOverlay.xul"?>
>+

Second, there's no need to add a new blank line here.  In fact, it might be worth *removing* one of the blank lines.



So r=dbaron... but could you post a revised patch with those changes, so that somebody can commit it?  And when you do, needinfo? one of us so we can make that happen.
Attachment #8948198 - Flags: review?(dbaron) → review+
Flags: needinfo?(dbaron)
Attachment #8948434 - Flags: review?(dbaron)
Attachment #8948434 - Flags: review?(dbaron) → review+
Attachment #8948198 - Attachment is obsolete: true
Attachment #8946605 - Attachment is obsolete: true
Keywords: checkin-needed
Pushed by dluca@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/aad65073f756
Remove references to old (SeaMonkey) overlays from layoutdebug code.  r=dbaron
Keywords: checkin-needed
Thanks for the patch!  It's landed now in mozilla-inbound, and should be in mozilla-central (and thus the nightly channel) sometime in the next 24 hours.
Flags: needinfo?(dbaron)
https://hg.mozilla.org/mozilla-central/rev/aad65073f756
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.