Closed Bug 648026 Opened 9 years ago Closed 9 years ago

Move Content Popup Helper out of FormHelperUI

Categories

(Firefox for Android Graveyard :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: vingtetun, Assigned: vingtetun)

References

Details

Attachments

(1 file, 2 obsolete files)

Attached patch Patch (obsolete) — Splinter Review
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-
Attached patch Patch v0.2 (obsolete) — Splinter Review
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
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
Closed: 9 years ago
Resolution: --- → FIXED
Can this be marked as verified fixed?
You need to log in before you can comment on or make changes to this bug.