Closed Bug 1054600 Opened 5 years ago Closed 5 years ago

Refactor AboutHomeListener & browser.js document URI checks (followup from bug 1041678)

Categories

(Firefox :: Search, defect)

34 Branch
defect
Not set
Points:
2

Tracking

()

RESOLVED FIXED
Firefox 34
Iteration:
34.3

People

(Reporter: alexbardas, Assigned: alexbardas)

References

Details

Attachments

(1 file, 3 obsolete files)

AboutHomeListener.onUpdate checks the document URI.  onFocusInput should, too.  That check should be factored out into its own method, and then both receiveMessage and handleEvent should call it so that all the various methods that they delegate to don't have to, and we don't get tripped up by this again.
Assignee: nobody → abardas
Blocks: 1041678
Status: NEW → ASSIGNED
Comment on attachment 8474046 [details] [diff] [review]
Refactor documentURI checks in AboutHomeListener and browser.js Patch #1

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js

>     let openSearchPageIfFieldIsNotActive = function(aSearchBar) {
>-      let doc = gBrowser.selectedBrowser.contentDocument;
>-      let url = doc.documentURI.toLowerCase();
>+      let url = gBrowser.currentURI.spec;

You probably want to keep the toLowerCase()
QA Whiteboard: [qa?]
Flags: firefox-backlog+
Keep .toLowerCase() logic
Attachment #8474046 - Attachment is obsolete: true
Attachment #8474046 - Flags: review?(adw)
Attachment #8474184 - Flags: review?(adw)
Comment on attachment 8474184 [details] [diff] [review]
Refactor documentURI checks in AboutHomeListener and browser.js Patch #2

>--- a/browser/base/content/content.js
>+++ b/browser/base/content/content.js

>+// Get the current document uri.
>+function getURI() {
>+  return content.document.documentURI.toLowerCase();
>+}

While you might not care about upper/lower case differences in about: URIs, other consumer might care for other URIs. Introducing what looks like a general-purpose utility function that silently makes all URIs lower case doesn't seem sane.
Comment on attachment 8474184 [details] [diff] [review]
Refactor documentURI checks in AboutHomeListener and browser.js Patch #2

Review of attachment 8474184 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/content.js
@@ +24,5 @@
>    return Services.io.newURI(uri, originCharset, baseURI);
>  }
>  
> +// Get the current document uri.
> +function getURI() {

I agree with Dao about this.  What I meant by factoring out is the uri == "about:home" check in AboutHomeListener, so there should be something like an AboutHomeListener.isAboutHome getter that AboutHomeListener calls internally, similar to the ContentSearchMediator._contentWhitelisted getter.

@@ +33,5 @@
>    // Hide session restore button on about:home
>    let doc = content.document;
>    let container;
> +  if (getURI() == "about:home" &&
> +      (container = doc.getElementById("sessionRestoreContainer"))) {

So here, it's fine to leave this as it is, no refactoring necessary.  Or, this could call AboutHomeListener.isAboutHome.

@@ +97,5 @@
>    init: function(chromeGlobal) {
>      chromeGlobal.addEventListener('AboutHomeLoad', () => this.onPageLoad(), false, true);
>    },
>  
>    handleEvent: function(aEvent) {

I didn't realize this earlier, but this method is never actually called because nobody adds an event listener with AboutHomeListener as the target.  It looks like a mistake because that's always been the case since bug 899222 landed the AboutHomeListener (http://hg.mozilla.org/mozilla-central/rev/0f68b32be429).

I think we should change all the addEventListener calls to pass `this` (AboutHomeListener) as the target so that all events are routed through here.  Then we can do the uri == "about:home" check here.  That means we need to move the pagehide and AboutHomeSearchEvent listener functions out of onPageLoad into their own top-level methods.

Does anybody disagree?
Attachment #8474184 - Flags: review?(adw)
QA Whiteboard: [qa?] → [qa-]
OS: Mac OS X → All
Hardware: x86 → All
Attachment #8474184 - Attachment is obsolete: true
Attachment #8474701 - Flags: review?(adw)
Comment on attachment 8474701 [details] [diff] [review]
Refactor documentURI checks in AboutHomeListener and browser.js Patch #3

Review of attachment 8474701 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, r+ with the changes below.

::: browser/base/content/content.js
@@ +89,5 @@
>  }
>  
>  let AboutHomeListener = {
>    init: function(chromeGlobal) {
>      chromeGlobal.addEventListener('AboutHomeLoad', () => this.onPageLoad(), false, true);

Need to make the target `this` instead of the anonymous function so that handleEvent is called.

@@ +110,5 @@
> +        break;
> +      case "pagehide":
> +        this.onPageHide(aEvent);
> +        break;
> +      case "AboutHomeSearchEvent":

Nit: please move this above the "click" case so that the cases are in alphabetical order.

@@ +148,5 @@
>    },
>  
>    onPageLoad: function() {
>      let doc = content.document;
> +    if (!this.isAboutHome || doc.documentElement.hasAttribute("hasBrowserHandlers")) {

The !this.isAboutHome term can be removed with the `this` target change I mentioned above.

@@ +239,3 @@
>    onFocusInput: function () {
> +    let searchInput = content.document.getElementById("searchText");
> +    if (searchInput) {

Now that we're properly ensuring that the page is about:home before calling this method, this conditional shouldn't be necessary since the input should always be in the page, but I guess it couldn't hurt to check, so I won't ask you to remove it.
Attachment #8474701 - Flags: review?(adw) → review+
https://hg.mozilla.org/mozilla-central/rev/976ddf70355a
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
Iteration: --- → 34.3
Points: --- → 2
QA Whiteboard: [qa-]
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.