Move Content Popup Helper out of FormHelperUI

RESOLVED FIXED

Status

Fennec Graveyard
General
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: vingtetun, Assigned: vingtetun)

Tracking

Dependency tree / graph

Details

Attachments

(1 attachment, 2 obsolete attachments)

Created attachment 524209 [details] [diff] [review]
Patch

The content popup code can be useful for others stuff too, and extension developers will be able to add things on top of it. Moving it out of FormHelperUI also means a simpler FormHelperUI code and a feature easier to test.
Attachment #524209 - Flags: review?(mark.finkle)
Comment on attachment 524209 [details] [diff] [review]
Patch

>diff --git a/chrome/content/ContentPopupHelper.js b/chrome/content/ContentPopupHelper.js

>+var ContentPopupHelper = {
>+  init: function() {
>+    window.addEventListener("TabSelect", this, false);
>+    window.addEventListener("TabClose", this, false);
>+    Elements.browsers.addEventListener("PanBegin", this, false);
>+    Elements.browsers.addEventListener("PanFinished", this, false);
>+    window.addEventListener("AnimatedZoomBegin", this, false);
>+    window.addEventListener("AnimatedZoomEnd", this, false);
>+    window.addEventListener("MozBeforeResize", this, true);
>+    window.addEventListener("resize", this, false);
>+  },

It looks like we can delay adding event listeners until the "popup" setter. We should remove the even listeners too, when popup = null

>+   * This method positionned an arrowbox on the screen using a 'virtual'

positioned

>+  update: function(aAnchorRect) {

if we are anchoring, why not call this "anchorTo" instead of "update" ?

>+      case "PanFinished":
>+      case "AnimatedZoomEnd":
>+        popup.style.visibility = "visible";
>+        //this.update();

remove the code if it's not needed

>diff --git a/chrome/content/browser-ui.js b/chrome/content/browser-ui.js
>       FindHelperUI.init();
>+      ContentPopupHelper.init();

No thank you. We should not need to init this unless a popup is being displayed, unless I am missing something.

>diff --git a/chrome/content/common-ui.js b/chrome/content/common-ui.js

>   _updateSuggestionsFor: function _formHelperUpdateAutocompleteFor(aElement) {

>     let suggestionsContainer = this._suggestionsContainer;
>-    suggestionsContainer.style.visibility = "hidden";
>-    suggestionsContainer.hidden = false;
>-    suggestionsContainer.left = 0;

Do we really need to keep suggestionsContainer? If we do, move it closer to the first use.

>diff --git a/chrome/tests/browser_autocomplete.js b/chrome/tests/browser_autocomplete.js

>-  newTab = Browser.addTab(testURL, true);
>+  let startupInfo = Cc["@mozilla.org/toolkit/app-startup;1"].getService(Ci.nsIAppStartup).getStartupInfo();
>+  if (!("firstPaint" in startupInfo))
>+    waitFor(function() { newTab = Browser.addTab(testURL, true); }, function() {
>+      let startupInfo = Cc["@mozilla.org/toolkit/app-startup;1"].getService(Ci.nsIAppStartup).getStartupInfo();
>+      return ("firstPaint" in startupInfo);
>+    }, Date.now() + 3000);
>+  else
>+    newTab = Browser.addTab(testURL, true);


Oh my. What is going on here?

>diff --git a/chrome/tests/browser_contentpopup.js b/chrome/tests/browser_contentpopup.js

>+  let startupInfo = Cc["@mozilla.org/toolkit/app-startup;1"].getService(Ci.nsIAppStartup).getStartupInfo();
>+  if (!("firstPaint" in startupInfo))
>+    waitFor(function() { newTab = Browser.addTab(testURL, true); }, function() {
>+      let startupInfo = Cc["@mozilla.org/toolkit/app-startup;1"].getService(Ci.nsIAppStartup).getStartupInfo();
>+      return ("firstPaint" in startupInfo);
>+    }, Date.now() + 3000);
>+  else
>+    newTab = Browser.addTab(testURL, true);
>+}

And here

r- for a new patch and answers to some questions
Attachment #524209 - Flags: review?(mark.finkle) → review-
Created attachment 524677 [details] [diff] [review]
Patch v0.2

Address comments.

I've not yet removed my 'firstPaint' hack for tests, for some reasons I need to wait for that on my laptop if I run the test alone to prevent random oranges.
I can remove it if needed, it won't affect the browser-chrome tests when run as a test suite.
Assignee: nobody → 21
Attachment #524209 - Attachment is obsolete: true
Attachment #524677 - Flags: review?(mark.finkle)
Mark,
I know you don't like this patch but if I removed the firstPaint hack will it be ok?
Still thinking about this. I'm trying to see the value in breaking it out. Got any examples?
(In reply to comment #4)
> Still thinking about this. I'm trying to see the value in breaking it out. Got
> any examples?

There is a few differents case where breaking it out make sense:
 * Code readability: FormHelperUI will be easier to handle and understand
 * Code maintenance: Having a separate helper will make life easier to test and maintain this code without any backside effect
 * Usable from the outside world: as an example I need it for the form validation html 5 bug but also extensions developer will be able to use that without coding their own "popup" helper
Blocks: 656373
Created attachment 532161 [details] [diff] [review]
Patch v0.2 - updated on trunk
Attachment #524677 - Attachment is obsolete: true
Attachment #524677 - Flags: review?(mark.finkle)
Attachment #532161 - Flags: review?(mark.finkle)
Attachment #532161 - Flags: review?(mark.finkle) → review+
http://hg.mozilla.org/mozilla-central/rev/f47778cd53b5
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED

Comment 8

7 years ago
Can this be marked as verified fixed?
You need to log in before you can comment on or make changes to this bug.