Closed Bug 1425363 Opened 6 years ago Closed 6 years ago

Switch mac window menu to JS instead of RDF

Categories

(Toolkit :: General, enhancement)

Product:
Toolkit
Component:
General
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: Gijs, Assigned: Gijs)

References

Details

Attachments

(3 files)

Bug 1425363 - use JS instead of the windowds data source for the mac window menu,
59 bytes, text/x-review-board-request
bgrins
: review+
spohl
: review+
Details
Bug 1425363 - remove the windowds XPCOM component,
59 bytes, text/x-review-board-request
spohl
: review+
Details
Bug 1425363 - remove onWindowTitleChanged given that nobody uses it,
59 bytes, text/x-review-board-request
bgrins
: review+
Details 
As far as we can tell, we have an RDF data source for the single purpose of doing this menu only on mac.

Instead we could just implement it with some simple JS.
(In reply to :Gijs from comment #4)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=5b68c3d47de6

Hrmpf, things built alright locally so I forgot about packaging, and missed removing the .xpt file from the installer.

In other news, looks like we build and ship this stuff on android!? Wat.
Comment on attachment 8936982 [details]
Bug 1425363 - use JS instead of the windowds data source for the mac window menu,

https://reviewboard.mozilla.org/r/207724/#review213654

browser/extensions/pdfjs/test/browser_pdfjs_savedialog.js seems to be failing on try. Could you verify that this isn't related to the changes here? Two minor nits below.

::: toolkit/content/macWindowMenu.js:8
(Diff revision 1)
>  function macWindowMenuDidShow() {
> -  var windowManagerDS =
> -    Components.classes["@mozilla.org/rdf/datasource;1?name=window-mediator"]
> -              .getService(Components.interfaces.nsIWindowDataSource);
> -  var sep = document.getElementById("sep-window-list");
>    // Using double parens to avoid warning

nit: remove comment

::: toolkit/content/macWindowMenu.js:23
(Diff revision 1)
> +    if (win == window) {
> +      item.setAttribute("checked", "true");
> -}
> +    }
> -
> -function toOpenWindow( aWindow ) {
> -  // deminiaturize the window, if it's in the Dock
> +    item.addEventListener("command", () => {
> +      if (win.windowState == window.STATE_MINIMIZED)
> +        win.restore();

nit: wrap in braces
Attachment #8936982 - Flags: review?(spohl.mozilla.bugs)
Comment on attachment 8936983 [details]
Bug 1425363 - remove the windowds XPCOM component,

https://reviewboard.mozilla.org/r/207726/#review213668
Attachment #8936983 - Flags: review?(spohl.mozilla.bugs) → review+
Comment on attachment 8936982 [details]
Bug 1425363 - use JS instead of the windowds data source for the mac window menu,

https://reviewboard.mozilla.org/r/207724/#review213676

::: toolkit/content/macWindowMenu.js:9
(Diff revision 1)
> -  var windowManagerDS =
> -    Components.classes["@mozilla.org/rdf/datasource;1?name=window-mediator"]
> -              .getService(Components.interfaces.nsIWindowDataSource);
> -  var sep = document.getElementById("sep-window-list");
>    // Using double parens to avoid warning
> -  while ((sep = sep.nextSibling)) {
> +  let windows = Services.wm.getEnumerator("");

Self-nit: need to check that we actually have `Services` everywhere we run this, and import it when we don't.
(In reply to Stephen A Pohl [:spohl] from comment #9)
> browser/extensions/pdfjs/test/browser_pdfjs_savedialog.js seems to be
> failing on try. Could you verify that this isn't related to the changes
> here? Two minor nits below.

Good catch! I'm pretty sure this is because of the third cset which touches the test (removes the empty onWindowTitleChanged handler), which triggers TV (test verification) on it, but I bet that TV fails for this test generally because the test is slightly bogus. I'll push a dummy cset with a whitespace change to the test to try to verify.

(This type of thing is part of why that test is still tier-2)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1ff985a37a5399320502af37160c11adb3808dc8
(In reply to :Gijs from comment #12)
> (In reply to Stephen A Pohl [:spohl] from comment #9)
> > browser/extensions/pdfjs/test/browser_pdfjs_savedialog.js seems to be
> > failing on try. Could you verify that this isn't related to the changes
> > here? Two minor nits below.
> 
> Good catch! I'm pretty sure this is because of the third cset which touches
> the test (removes the empty onWindowTitleChanged handler), which triggers TV
> (test verification) on it, but I bet that TV fails for this test generally
> because the test is slightly bogus. I'll push a dummy cset with a whitespace
> change to the test to try to verify.
> 
> (This type of thing is part of why that test is still tier-2)
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=1ff985a37a5399320502af37160c11adb3808dc8

Yep, this is also all orange.
Comment on attachment 8936982 [details]
Bug 1425363 - use JS instead of the windowds data source for the mac window menu,

https://reviewboard.mozilla.org/r/207724/#review213694

Thanks for verifying.
Attachment #8936982 - Flags: review+
Comment on attachment 8936982 [details]
Bug 1425363 - use JS instead of the windowds data source for the mac window menu,

https://reviewboard.mozilla.org/r/207724/#review213940

I love the fact that it takes less (and more straightforward) code to implement this feature in vanilla JS
Attachment #8936982 - Flags: review?(bgrinstead) → review+
Comment on attachment 8936984 [details]
Bug 1425363 - remove onWindowTitleChanged given that nobody uses it,

https://reviewboard.mozilla.org/r/207728/#review213942

r+!
Attachment #8936984 - Flags: review?(bgrinstead) → review+
Comment on attachment 8936982 [details]
Bug 1425363 - use JS instead of the windowds data source for the mac window menu,

https://reviewboard.mozilla.org/r/207724/#review214000

::: toolkit/content/macWindowMenu.js:9
(Diff revision 1)
> -  var windowManagerDS =
> -    Components.classes["@mozilla.org/rdf/datasource;1?name=window-mediator"]
> -              .getService(Components.interfaces.nsIWindowDataSource);
> -  var sep = document.getElementById("sep-window-list");
>    // Using double parens to avoid warning
> -  while ((sep = sep.nextSibling)) {
> +  let windows = Services.wm.getEnumerator("");

This is fine because this stuff is included in baseMenuOverlay.xul which loads utilityOverlay.js which imports Services.jsm.
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/8cc8bd064aa8
use JS instead of the windowds data source for the mac window menu, r=bgrins,spohl
https://hg.mozilla.org/integration/autoland/rev/f8a8b592c702
remove the windowds XPCOM component, r=spohl
https://hg.mozilla.org/integration/autoland/rev/59321d42d617
remove onWindowTitleChanged given that nobody uses it, r=bgrins
Blocks: 1426001
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/e2167dafd13a
Port bug 1425363 to TB/IB/SM: remove windowds.xpt from package manifests. rs=bustage-fix
Blocks: SM-killrdf
No longer blocks: SM-killrdf
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: