+++ This bug was initially created as a clone of Bug #1480982 +++ In PanelMultiView.jsm, every place where _dwu is used ends up calling getBoundsWithoutFlushing: foo = this._dwu.getBoundsWithoutFlushing(...); So let's replace this: https://searchfox.org/mozilla-central/rev/51268dcbdff0f6f4a5cff7986df0f616efc5bcfd/browser/components/customizableui/PanelMultiView.jsm#175-182 with a new _getBoundsWithoutFlushing method: _getBoundsWithoutFlushing(element) { return this.window.windowUtils.getBoundsWithoutFlushing(element); }, and replace the _dwu users with a call to our new method: foo = this._getBoundsWithoutFlushing(...);

I want to work on this issue.

(In reply to sahilbhosale63 from comment #1) > I want to work on this issue. Go for it! Do you have the Firefox source code already? Let me know if you have questions.

Yes, I already have the source code running on my machine.

Bug 1481401 - replace _dwr getter with _getBoundsWithoutFlushing method. r?dao

Comment on attachment 8998425 [details] [diff] [review] tip.patch Next time when attaching a patch that you want to get reviewed, please make sure to set the review flag. >@@ -1368,7 +1368,7 @@ var PanelView = class extends Associated > ".subviewbutton:not([disabled]), .subviewkeynav:not([disabled])")); > let dwu = this._dwu; > return buttons.filter(button => { >- let bounds = dwu.getBoundsWithoutFlushing(button); >+ let bounds = _getBoundsWithoutFlushing(button); > return bounds.width > 0 && bounds.height > 0; > }); You need to remove the dwu variable and call this._getBoundsWithoutFlushing rather than _getBoundsWithoutFlushing in the filter function. You also still need to replace the _dwu implementation (https://searchfox.org/mozilla-central/rev/51268dcbdff0f6f4a5cff7986df0f616efc5bcfd/browser/components/customizableui/PanelMultiView.jsm#175-182) with the _getBoundsWithoutFlushing implementation I posted in comment 0.

Bug 1481401- replaced -dwu getter with _getBoundsWithoutFlushing method. r?dao

Comment on attachment 8998453 [details] [diff] [review] tip.patch > /** > * nsIDOMWindowUtils for the window of this node. > */ This comment is now obsolete, please remove it. >- get _dwu() { >- if (this.__dwu) >- return this.__dwu; >- return this.__dwu = this.window.QueryInterface(Ci.nsIInterfaceRequestor) >+ get _getBoundsWithoutFlushing() { >+ if (this.__getBoundsWithoutFlushing) >+ return this.__getBoundsWithoutFlushing; >+ return this.__getBoundsWithoutFlushing = this.window.QueryInterface(Ci.nsIInterfaceRequestor) > .getInterface(Ci.nsIDOMWindowUtils); > } No need for __getBoundsWithoutFlushing, just return this.window.windowUtils directly. > >@@ -424,7 +424,7 @@ var PanelMultiView = class extends Assoc > this._panel.removeEventListener("popuphidden", this); > this.window.removeEventListener("keydown", this); > this.node = this._openPopupPromise = this._openPopupCancelCallback = >- this._viewContainer = this._viewStack = this.__dwu = >+ this._viewContainer = this._viewStack = this.__getBoundsWithoutFlushing = > this._transitionDetails = null; > } Please remove __getBoundsWithoutFlushing here as well. >@@ -1366,9 +1366,9 @@ var PanelView = class extends Associated > getNavigableElements() { > let buttons = Array.from(this.node.querySelectorAll( > ".subviewbutton:not([disabled]), .subviewkeynav:not([disabled])")); >- let dwu = this._dwu; >+ nit: remove spaces from the empty line

Comment on attachment 8998460 [details] [diff] [review] removed the comments and blank spaces, and changed the things which you said. >--- a/browser/components/customizableui/PanelMultiView.jsm >+++ b/browser/components/customizableui/PanelMultiView.jsm >@@ -175,10 +175,10 @@ var AssociatedToNode = class { >- get _dwu() { >- if (this.__dwu) >- return this.__dwu; >- return this.__dwu = this.window.QueryInterface(Ci.nsIInterfaceRequestor) >+ get _getBoundsWithoutFlushing() { >+ if (this.__getBoundsWithoutFlushing) >+ return this.__getBoundsWithoutFlushing; >+ return this.window.QueryInterface(Ci.nsIInterfaceRequestor) > .getInterface(Ci.nsIDOMWindowUtils); > } The _dwu getter doesn't contain the QueryInterface anymore. Is your mozilla-central copy outdated? >@@ -424,7 +424,7 @@ var PanelMultiView = class extends Assoc > this._panel.removeEventListener("popuphidden", this); > this.window.removeEventListener("keydown", this); > this.node = this._openPopupPromise = this._openPopupCancelCallback = >- this._viewContainer = this._viewStack = this.__dwu = >+ this._viewContainer = this._viewStack = this._transitionDetails = null; > } This looks broken, _transitionDetails seems to be coming out of nowhere. Did you manually edit the patch? Btw, please set the review flag to ? rather than +.

I by mistake edited the tip.patch file directly, sorry for that. Now, I have . changed the source file and have done what you said to do.

Comment on attachment 8998464 [details] [diff] [review] tip.patch >diff --git a/browser/components/customizableui/PanelMultiView.jsm b/browser/components/customizableui/PanelMultiView.jsm >--- a/browser/components/customizableui/PanelMultiView.jsm >+++ b/browser/components/customizableui/PanelMultiView.jsm >@@ -172,13 +172,10 @@ var AssociatedToNode = class { > return this.node.ownerGlobal; > } > >- /** >- * nsIDOMWindowUtils for the window of this node. >- */ >- get _dwu() { >- if (this.__dwu) >- return this.__dwu; >- return this.__dwu = this.window.QueryInterface(Ci.nsIInterfaceRequestor) >+ get _getBoundsWithoutFlushing() { >+ if (this.__getBoundsWithoutFlushing) >+ return this.__getBoundsWithoutFlushing; >+ return this.window.QueryInterface(Ci.nsIInterfaceRequestor) > .getInterface(Ci.nsIDOMWindowUtils); > } See my previous comment -- your mozilla-central copy looks outdated. You can update it with hg pull -u. > getNavigableElements() { > let buttons = Array.from(this.node.querySelectorAll( > ".subviewbutton:not([disabled]), .subviewkeynav:not([disabled])")); >- let dwu = this._dwu; >+ Please remove the spaces from the empty line.

Updated my source code and removed the extra space.

Comment on attachment 8998468 [details] [diff] [review] tip.patch >- /** >- * nsIDOMWindowUtils for the window of this node. >- */ >- get _dwu() { >- if (this.__dwu) >- return this.__dwu; >- return this.__dwu = this.window.QueryInterface(Ci.nsIInterfaceRequestor) >+ get _getBoundsWithoutFlushing() { >+ if (this.__getBoundsWithoutFlushing) >+ return this.__getBoundsWithoutFlushing; >+ return this.window.QueryInterface(Ci.nsIInterfaceRequestor) > .getInterface(Ci.nsIDOMWindowUtils); > } This is still not up to date. The _dwu getter should have looked like this: /** * nsIDOMWindowUtils for the window of this node. */ get _dwu() { if (this.__dwu) return this.__dwu; return this.__dwu = this.window.windowUtils; }

I have changed _dwu with _getBoundsWithoutFlushing because i thought i also have to change it.

(In reply to Sahil Bhosale from comment #14) > Created attachment 8998471 [details] [diff] [review] > final.patch > > I have changed _dwu with _getBoundsWithoutFlushing because i thought i also > have to change it. Again, the goal is to replace this: > /** > * nsIDOMWindowUtils for the window of this node. > */ > get _dwu() { > if (this.__dwu) > return this.__dwu; > return this.__dwu = this.window.windowUtils; > } With this: > _getBoundsWithoutFlushing(element) { > return this.window.windowUtils.getBoundsWithoutFlushing(element); > },

Comment on attachment 8998534 [details] [diff] [review] replaced the dwu method with _getBoundsWithoutFlushing >@@ -1366,13 +1359,11 @@ var PanelView = class extends Associated > getNavigableElements() { > let buttons = Array.from(this.node.querySelectorAll( > ".subviewbutton:not([disabled]), .subviewkeynav:not([disabled])")); >- let dwu = this._dwu; > return buttons.filter(button => { >- let bounds = _getBoundsWithoutFlushing(button); I don't see where this line is coming from... Did you manually edit the patch file? > return bounds.width > 0 && bounds.height > 0; > }); > } >- > /** Please leave the empty line.

No, I have not edited it manually.

Maybe you committed some other changes then, before creating the patch. Your patch says you removed this line: >- let bounds = _getBoundsWithoutFlushing(button); But PanelMultiView.jsm doesn't have that line in the first place. What it does have is this line: > let bounds = dwu.getBoundsWithoutFlushing(element); https://searchfox.org/mozilla-central/rev/0f4d50ff5211e8dd85e119ef683d04b63062ed90/browser/components/customizableui/PanelMultiView.jsm#1382

I solved that extra space problem. The reason why that space was there is because i haven't left an empty line between the code and the comment. I think now it is done.

Comment on attachment 8998775 [details] [diff] [review] final.patch Okay, this is starting to look really good, except... is your mozilla-central copy still outdated? For instance, you patched this method: > getNavigableElements() { > let buttons = Array.from(this.node.querySelectorAll( > ".subviewbutton:not([disabled]), .subviewkeynav:not([disabled])")); >- let dwu = this._dwu; > return buttons.filter(button => { >- let bounds = _getBoundsWithoutFlushing(button); >+ let bounds = this._getBoundsWithoutFlushing(button); > return bounds.width > 0 && bounds.height > 0; > }); > } But getNavigableElements doesn't exist anymore. There's a _navigableElements getter instead: https://searchfox.org/mozilla-central/rev/0f4d50ff5211e8dd85e119ef683d04b63062ed90/browser/components/customizableui/PanelMultiView.jsm#1366

(In reply to Dão Gottwald [::dao] from comment #21) > Comment on attachment 8998775 [details] [diff] [review] > final.patch > > Okay, this is starting to look really good, except... is your > mozilla-central copy still outdated? > > For instance, you patched this method: > > > getNavigableElements() { > > let buttons = Array.from(this.node.querySelectorAll( > > ".subviewbutton:not([disabled]), .subviewkeynav:not([disabled])")); > >- let dwu = this._dwu; > > return buttons.filter(button => { > >- let bounds = _getBoundsWithoutFlushing(button); > >+ let bounds = this._getBoundsWithoutFlushing(button); > > return bounds.width > 0 && bounds.height > 0; > > }); > > } > > But getNavigableElements doesn't exist anymore. There's a _navigableElements > getter instead: > > https://searchfox.org/mozilla-central/rev/ > 0f4d50ff5211e8dd85e119ef683d04b63062ed90/browser/components/customizableui/ > PanelMultiView.jsm#1366 I have updated my mozilla-centeral now using hg pull -u command but still it shows me the getNavigationElements() method instead of _navigationElements. What should I do?

Did hg pull -u succeed? What was its output?

This was the output of hg pull -u command: pulling from https://hg.mozilla.org/mozilla-central searching for changes adding changesets adding manifests adding file changes added 323 changesets with 2001 changes to 1379 files new changesets ec96693b39be:f650c0df72f9 0 files updated, 0 files merged, 0 files removed, 0 files unresolved updated to "3dd40d95c802: Bug 1481401 - replace _dwu getter with _getBoundsWithoutFlushing method. r?dao" 1 other heads for branch "default" Here it is saying that 0 files updated and I don't know why it is mentioning the bug no.

You created another head because you had a local commit before updating. Maybe try: hg strip 3dd40d95c802 && hg pull && hg up

(In reply to Dão Gottwald [::dao] from comment #25) > You created another head because you had a local commit before updating. > Maybe try: > > hg strip 3dd40d95c802 && hg pull && hg up It says unknown command 'strip' see this: hg: unknown command 'strip' 'strip' is provided by the following extension: strip strip changesets and their descendants from history (use 'hg help extensions' for information on enabling extensions)

What does hg heads output?

(In reply to Dão Gottwald [::dao] from comment #27) > What does hg heads output? changeset: 430751:f650c0df72f9 tag: tip parent: 430595:eb9ff7de69ef parent: 430750:21cbd16b3b27 user: Tiberius Oros <toros@mozilla.com> date: Thu Aug 09 13:02:05 2018 +0300 summary: Merge inbound to mozilla-central. a=merge changeset: 422984:3dd40d95c802 user: Sahil <sahilbhosale63@live.com> date: Wed Aug 08 00:21:35 2018 +0530 summary: Bug 1481401 - replace _dwu getter with _getBoundsWithoutFlushing method. r?dao

Ah, you didn't have the strip extension enabled. Open your ~/.hgrc file and add a line saying "strip = " (without quotes) under [extensions], then try the command from comment 25 again.

I my system when i run: 1. ~/.hgrc : It gives output permission denied. 2. hg config -e : It runs successfully and I typed the (strip = ) in the [extensions]. 3. hg config -e -g : It gives the output no such file or directory. After running the 2. command shown above, I ran the command in comment 25 i.e: hg strip 3dd40d95c802 && hg pull && hg up which gives following output: abort: local changes found

(In reply to Sahil Bhosale from comment #30) > I my system when i run: > > 1. ~/.hgrc : It gives output permission denied. It's a text file, you'd need to open it with a text editor rather than executing it. > 2. hg config -e : It runs successfully and I typed the (strip = ) in the > [extensions]. > > 3. hg config -e -g : It gives the output no such file or directory. > > > After running the 2. command shown above, I ran the command in comment 25 > i.e: hg strip 3dd40d95c802 && hg pull && hg up which gives following output: > > abort: local changes found Try: hg up -C && strip 3dd40d95c802 && hg pull && hg up Note that this will remove your local changes.

> Try: > > hg up -C && strip 3dd40d95c802 && hg pull && hg up > > Note that this will remove your local changes. This was the output for the above command: 0 files updated, 0 files merged, 0 files removed, 0 files unresolved updated to "3dd40d95c802: Bug 1481401 - replace _dwu getter with _getBoundsWithoutFlushing method. r?dao" 1 other heads for branch "default" strip: '3dd40d95c802': No such file

What is the progress of this bug does it has been solved or some things are still remaining?

there's "hg" missing before "strip" try this: hg strip 3dd40d95c802 && hg pull && hg up

Updated the source code and replaced the dwu method.

replaced all the dwu method with getBoundsWithoutFlushing().

Comment on attachment 8998860 [details] [diff] [review] final.patch >@@ -423,8 +418,7 @@ var PanelMultiView = class extends Assoc > this._panel.removeEventListener("popuphidden", this); > this.window.removeEventListener("keydown", this); > this.node = this._openPopupPromise = this._openPopupCancelCallback = >- this._viewContainer = this._viewStack = this.__dwu = >- this._transitionDetails = null; >+ this._viewContainer = this._viewStack = this._transitionDetails = null; > } > > /** >@@ -878,7 +872,7 @@ var PanelMultiView = class extends Assoc > let header = viewNode.firstChild; > if (header && header.classList.contains("panel-header")) { > viewRect.height += await window.promiseDocumentFlushed(() => { >- return this._dwu.getBoundsWithoutFlushing(header).height; >+ return this.getBoundsWithoutFlushing(header).height; > }); > } > await nextPanelView.descriptionHeightWorkaround(); >@@ -892,7 +886,7 @@ var PanelMultiView = class extends Assoc > await nextPanelView.descriptionHeightWorkaround(); > > viewRect = await window.promiseDocumentFlushed(() => { >- return this._dwu.getBoundsWithoutFlushing(viewNode); >+ return this.getBoundsWithoutFlushing(viewNode); > }); > // Bail out if the panel was closed in the meantime. > if (!nextPanelView.isOpenIn(this)) { >@@ -1257,7 +1251,7 @@ var PanelView = class extends Associated > * navigation if the view is still open but is invisible. > */ > captureKnownSize() { >- let rect = this._dwu.getBoundsWithoutFlushing(this.node); >+ let rect = this.getBoundsWithoutFlushing(this.node); > this.knownWidth = rect.width; > this.knownHeight = rect.height; > } >@@ -1370,7 +1364,6 @@ var PanelView = class extends Associated > > let navigableElements = Array.from(this.node.querySelectorAll( > ":-moz-any(button,toolbarbutton,menulist,.text-link):not([disabled])")); >- let dwu = this._dwu; > return this.__navigableElements = navigableElements.filter(element => { > // Set the "tabindex" attribute to make sure the element is focusable. > if (!element.hasAttribute("tabindex")) { >@@ -1379,7 +1372,7 @@ var PanelView = class extends Associated > if (element.hasAttribute("disabled")) { > return false; > } >- let bounds = dwu.getBoundsWithoutFlushing(element); >+ let bounds = this.getBoundsWithoutFlushing(element); > return bounds.width > 0 && bounds.height > 0; > }); > } These should all call this._getBoundsWithoutFlushing rather than this.getBoundsWithoutFlushing.

Changes done.

Comment on attachment 8998898 [details] [diff] [review] final.patch > return this.node.ownerGlobal; > } > >- /** >- * nsIDOMWindowUtils for the window of this node. >- */ >- get _dwu() { >- if (this.__dwu) >- return this.__dwu; >- return this.__dwu = this.window.windowUtils; >- } >+ _getBoundsWithoutFlushing(element) { >+ return this.window.windowUtils._getBoundsWithoutFlushing(element); Here you need to call this.window.windowUtils.getBoundsWithoutFlushing, not this.window.windowUtils._getBoundsWithoutFlushing.

Done, now the toolbar is working properly.

Comment on attachment 8998956 [details] [diff] [review] final.patch Looks good, thanks!

Can you just guide me here, what I have to do next or is my job done?

Hi Sahil, dao has reviewed your patch and given it an r+, and has also set the checkin-needed flag. That means that, within a few hours, someone will come along and land your patch for you and eventually (assuming the tests pass in our continuous integration branch), this bug will be marked RESOLVED FIXED. So at this point, your job is done! Congrats! :)

Pushed by nerli@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/435661644b2d "PanelMultiView.jsm: replace _dwu getter with _getBoundsWithoutFlushing method" [r=dao+bmo]

Backed out changeset 435661644b2d (bug 1481401) For Eslint failure on PanelMultiView.jsm CLOSED TREE Backout revision https://hg.mozilla.org/integration/mozilla-inbound/rev/83ab70ff5231b11e46b72f7f34579f4962c48661 Failed push: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=435661644b2d2448563a71b4d5599278ed68f6b5&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=193144962&repo=mozilla-inbound&lineNumber=252

Sahil, your patch still had trailing spaces and so it didn't land successfully. Can you please remove these spaces? + _getBoundsWithoutFlushing(element) { + return this.window.windowUtils.getBoundsWithoutFlushing(element); + } <--

(In reply to Dão Gottwald [::dao] from comment #46) > Sahil, your patch still had trailing spaces and so it didn't land > successfully. Can you please remove these spaces? > > + _getBoundsWithoutFlushing(element) { > + return this.window.windowUtils.getBoundsWithoutFlushing(element); > + } <-- Yes, I will soon remove the trailing spaces, I was in college so haven't done that. Now I will do that.

Removed the trailing space.

Pushed by cbrindusan@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/4d0483ecd593 PanelMultiView.jsm: replace _dwu getter with _getBoundsWithoutFlushing method. r=dao