Closed Bug 1434018 Opened 4 years ago Closed 4 years ago

Remove references to non-existent overlay


(Core :: Layout, enhancement)

Not set



Tracking Status
firefox60 --- fixed


(Reporter: bdahl, Assigned: dvabhinav31)




(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]

I'm not a peer or owner the layout module. You'll need someone from this list:
Attachment #8946605 - Flags: review?(bdahl)
Assignee: nobody → dvabhinav31
Comment on attachment 8946605 [details] [diff] [review]

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]

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]

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: .  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
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)
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.