Closed Bug 1481401 Opened 2 years ago Closed 2 years ago

PanelMultiView.jsm: replace _dwu getter with _getBoundsWithoutFlushing method

Categories

(Firefox :: Toolbars and Customization, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
Firefox 63
Tracking Status
firefox63 --- fixed

People

(Reporter: dao, Assigned: sahilbhosale63, Mentored)

References

Details

(Keywords: good-first-bug, Whiteboard: [lang=js])

Attachments

(1 file, 12 obsolete files)

+++ 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.
Attached patch tip.patch (obsolete) — Splinter Review
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.
Assignee: nobody → sahilbhosale63
Attached patch tip.patch (obsolete) — Splinter Review
Bug 1481401- replaced -dwu getter with _getBoundsWithoutFlushing method. r?dao
Attachment #8998453 - Flags: review+
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
Attachment #8998453 - Flags: review+
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 +.
Attachment #8998460 - Flags: review+
Attached patch tip.patch (obsolete) — Splinter Review
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.
Attachment #8998464 - Flags: review?(dao+bmo)
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.
Attachment #8998464 - Flags: review?(dao+bmo)
Attached patch tip.patch (obsolete) — Splinter Review
Updated my source code and removed the extra space.
Attachment #8998468 - Flags: review?(dao+bmo)
Attachment #8998425 - Attachment is obsolete: true
Attachment #8998464 - Attachment is obsolete: true
Attachment #8998453 - Attachment is obsolete: true
Attachment #8998460 - Attachment is obsolete: true
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;
  }
Attachment #8998468 - Flags: review?(dao+bmo)
Attached patch final.patch (obsolete) — Splinter Review
I have changed _dwu with _getBoundsWithoutFlushing because i thought i also have to change it.
Attachment #8998471 - Flags: review?(dao+bmo)
(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);
>  },
Attachment #8998468 - Attachment is obsolete: true
Attachment #8998471 - Flags: review?(dao+bmo)
Attachment #8998534 - Flags: review?(dao+bmo)
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.
Attachment #8998534 - Flags: review?(dao+bmo)
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
Attached patch final.patch (obsolete) — Splinter Review
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.
Attachment #8998775 - Flags: review?(dao+bmo)
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
Attachment #8998775 - Flags: review?(dao+bmo)
Attachment #8998471 - Attachment is obsolete: true
Attachment #8998534 - Attachment is obsolete: true
(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
Attached patch final.patch (obsolete) — Splinter Review
Updated the source code and replaced the dwu method.
Attachment #8998849 - Flags: review?(dao+bmo)
Attached patch final.patch (obsolete) — Splinter Review
replaced all the dwu method with getBoundsWithoutFlushing().
Attachment #8998860 - Flags: review?(dao+bmo)
Attachment #8998849 - Attachment is obsolete: true
Attachment #8998849 - Flags: review?(dao+bmo)
Attachment #8998775 - Attachment is obsolete: true
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.
Attachment #8998860 - Flags: review?(dao+bmo)
Attached patch final.patch (obsolete) — Splinter Review
Changes done.
Attachment #8998898 - Flags: review?(dao+bmo)
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.
Attachment #8998898 - Flags: review?(dao+bmo)
Attachment #8998860 - Attachment is obsolete: true
Attached patch final.patch (obsolete) — Splinter Review
Done, now the toolbar is working properly.
Attachment #8998956 - Flags: review?(dao+bmo)
Comment on attachment 8998956 [details] [diff] [review]
final.patch

Looks good, thanks!
Attachment #8998956 - Flags: review?(dao+bmo) → review+
Attachment #8998898 - Attachment is obsolete: true
Keywords: checkin-needed
Can you just guide me here, what I have to do next or is my job done?
Flags: needinfo?(dao+bmo)
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! :)
Flags: needinfo?(dao+bmo)
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]
Keywords: checkin-needed
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.
Flags: needinfo?(sahilbhosale63)
Attached patch final.patchSplinter Review
Removed the trailing space.
Attachment #8999138 - Flags: review?(dao+bmo)
Attachment #8999138 - Flags: review?(dao+bmo) → review+
Attachment #8998956 - Attachment is obsolete: true
Keywords: checkin-needed
Pushed by cbrindusan@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d0483ecd593
PanelMultiView.jsm: replace _dwu getter with _getBoundsWithoutFlushing method. r=dao
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4d0483ecd593
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
You need to log in before you can comment on or make changes to this bug.