648026 - Move Content Popup Helper out of FormHelperUI

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: 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.

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

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

Comment 2

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: 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.

Comment 3

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Mark,
I know you don't like this patch but if I removed the firstPaint hack will it be ok?

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

Still thinking about this. I'm trying to see the value in breaking it out. Got any examples?

Comment 5

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(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

Comment 6

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch v0.2 - updated on trunk

Comment 7

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

http://hg.mozilla.org/mozilla-central/rev/f47778cd53b5

Comment 8

[Andreea Pod]

Can this be marked as verified fixed?